Code review comment for lp:~brian.curtin/ubuntuone-client/851810-notify-on-volumes

Test are not running on linux because:

  File "/home/nessita/canonical/client/review_851810-notify-on-volumes/ubuntuone/platform/linux/dbus_interface.py", line 426, in Shares
    signature='a{ss}')
  File "/usr/lib/python2.7/dist-packages/dbus/decorators.py", line 333, in decorator
    raise ValueError('signal signature is longer than the number of arguments provided')
ValueError: signal signature is longer than the number of arguments provided
make: *** [test] Error 1

This is caused by the following:

73 + @dbus.service.signal(DBUS_IFACE_SHARES_NAME,
74 + signature='a{ss}')
75 + def VolumesReady(self):
76 + """Volumes are ready."""
77 +

VolumesReady should accept a param named 'volumes', like the signature specifies.

Also, in DBus you can't pass an array of objects, so you need to serialize the resulting volumes list. In IRL this means that with this branch we're getting this failure in the logs:

2012-03-07 12:49:39,359 - ubuntuone.SyncDaemon.EQ - ERROR - Error encountered while handling: VM_VOLUMES_READY in <ubuntuone.syncdaemon.interaction_interfaces.SyncdaemonEventListener object at 0xb501ed0>
Traceback (most recent call last):
  File "/home/nessita/canonical/client/review_851810-notify-on-volumes/ubuntuone/syncdaemon/event_queue.py", line 304, in _dispatch
    method(**kwargs)
  File "/home/nessita/canonical/client/review_851810-notify-on-volumes/ubuntuone/logger.py", line 269, in inner
    res = f(*args, **kwargs)
  File "/home/nessita/canonical/client/review_851810-notify-on-volumes/ubuntuone/syncdaemon/interaction_interfaces.py", line 1044, in handle_VM_VOLUMES_READY
    self.interface.shares.VolumesReady(volumes)
  File "/usr/lib/python2.7/dist-packages/dbus/decorators.py", line 314, in emit_signal
    message.append(signature=signature, *args)
TypeError: list indices must be integers, not RootVolume

So, in ubuntuone/syncdaemon/interaction_interfaces.py", line 1044, in handle_VM_VOLUMES_READY, you need to serialize the 'volumes' content. To do so, please use get_share_dict to serialize a the root and shares, and get_udf_dict for folders.

There is another thing to fix: the current volumes list has objects which classes are RooVolume, ShareVolume, and UDFVolume. These classes come from the protocol directly and are "low level", and should not be leaked to other modules. The volume_manager python module has a higher level abstraction for those, see the class Root, Shares and UDF (all 3 inheriting from Volume).

So, the volumes list send in the VM_VOLUMES_READY event should be a list of specializations of volume_manager.Volume. The fix does not ends there :-), as you can see in the

    549 def server_rescan(self):

method, after self._volumes_rescan_cb is done, another call is being to cleanup dead local volumes, so the event VM_VOLUMES_READY should be emitted once that cleanup is completed. Ergo, to solve both of these items (having high-level volume abstractions and a fully updated volumes list), I would suggest removing this line:

    639 events.append(("VM_VOLUMES_READY", dict(volumes=volumes)))

and adding a callback to the 'd' deferred inside server_rescan that will push the event once the cleanup deferred is fired. Applying that and converting server_rescan to use inlineCallbacks, we get something like this:

    549 @defer.inlineCallbacks
    550 def server_rescan(self):
    551 """Do the 'server rescan'"""
    552 volumes = yield self.m.action_q.query_volumes()
    553 shares, udfs = yield self._volumes_rescan_cb(volumes)
    554
    555 try:
    556 yield self._cleanup_volumes(shares=shares, udfs=udfs)
    557 except Exception, e:
    558 self.m.event_q.push('SYS_SERVER_RESCAN_ERROR', error=str(e))
    559 else:
    560 self.m.event_q.push('SYS_SERVER_RESCAN_DONE')
    561
    562 volumes = self.shares + self.udfs
    563 self.m.event_q.push("VM_VOLUMES_READY", volumes=volumes)
    564
    565 # refresh the pending shares and shared dirs list
    566 self.refresh_shares()

(blank spaces may be destroyed by launchpad, sorry)

What do you think? Does it make sense?