Merge lp:~cr3/checkbox/remove_storage_watcher_refactor into lp:checkbox

[Marc Tardif]

[Brendan Donegan (brendan-donegan)]

They aren't required options, they're required arguments ;) If you want to leave this change as it is then the jobs need to be updated to say:

removable_storage_watcher insert usb

instead of:

removable_storage_watcher --device=usb --action=insert

Personally I think the latter is clearer.

The other changes are fine though. Thanks!

review: Needs Fixing

[lp:~cr3/checkbox/remove_storage_watcher_refactor]

1188. By Marc Tardif on 2012-01-20

Updated jobs to match changed arguments in removable_storage_watcher.

[Marc Tardif (cr3)]

Good catch, I updated the jobs accordingly and resubmitting. The new syntax seems readable though, it reminds me of apt-get install foo or bzr pull bar. In other words, the first argument is an action and the second argument a target on which to perform the action.

review: Needs Resubmitting

[Brendan Donegan (brendan-donegan)]

In the interest of moving things forward, I'll not argue with that logic. Since the changes you made do seem to leave the script functionally the same, I'll merge this. I have noticed a few undesirable glitches but these were present before the refactoring too, so I'll sort them out in good time.

review: Approve

=== modified file 'jobs/firewire.txt.in'
--- jobs/firewire.txt.in	2012-01-20 16:23:54 +0000
+++ jobs/firewire.txt.in	2012-01-20 22:07:30 +0000
@@ -1,6 +1,6 @@
 plugin: manual
 name: firewire/insert
-command: removable_storage_watcher --device=firewire --action=insert
+command: removable_storage_watcher insert firewire
 _description:
  PURPOSE:
      This test will check the system can detect the insertion of a FireWire HDD
@@ -23,7 +23,7 @@
 plugin: manual
 name: firewire/remove
 depends: firewire/storage-test
-command: removable_storage_watcher --device=firewire --action=remove
+command: removable_storage_watcher remove firewire
 _description:
   PURPOSE:
     This test will check the system can detect the removal of a FireWire HDD

=== modified file 'jobs/usb.txt.in'
--- jobs/usb.txt.in	2012-01-20 10:50:13 +0000
+++ jobs/usb.txt.in	2012-01-20 22:07:30 +0000
@@ -51,7 +51,7 @@
 name: usb/insert
 depends: usb/detect
 requires: package.name == 'linux'
-command: removable_storage_watcher --device=usb --action=insert
+command: removable_storage_watcher insert usb
 _description:
  PURPOSE:
      This test will check that the system correctly detects the insertion of
@@ -68,7 +68,7 @@
 name: usb/remove
 depends: usb/storage-automated
 requires: package.name == 'linux'
-command: removable_storage_watcher --device=usb --action=remove
+command: removable_storage_watcher remove usb
 _description:
  PURPOSE:
      This test will check that the system correctly detects the removal of

=== modified file 'scripts/removable_storage_watcher'
--- scripts/removable_storage_watcher	2012-01-20 10:40:36 +0000
+++ scripts/removable_storage_watcher	2012-01-20 22:07:30 +0000
@@ -1,25 +1,43 @@
 #!/usr/bin/python
 
 import sys
-import signal
 import gobject
 import dbus
 
 from argparse import ArgumentParser
 from dbus.mainloop.glib import DBusGMainLoop
 
+
 class StorageDeviceListener(object):
 
-    def timeout(self):
-        print "Ten seconds has expired waiting for the device to be inserted."
-        self.error = 1
-        self.loop.quit()
+    def __init__(self, action, device):
+        self._action = action
+        self._device = device
+        self._bus = dbus.SystemBus(mainloop=DBusGMainLoop())
+        self._loop = gobject.MainLoop()
+
+    def check(self, timeout):
+    	if self._action == 'insert':
+        	self._bus.add_signal_receiver(self.add_detected, signal_name='DeviceAdded', dbus_interface='org.freedesktop.UDisks')
+    	elif self._action == 'remove':
+        	self._bus.add_signal_receiver(self.remove_detected, signal_name='DeviceRemoved', dbus_interface='org.freedesktop.UDisks')
+
+        error = False
+        def timeout_callback():
+            print "%s seconds have expired waiting for the device to be inserted." % timeout
+            global error
+            error = True
+            self._loop.quit()
+
+        gobject.timeout_add_seconds(timeout, timeout_callback)
+        self._loop.run()
+
+        return error
 
     def job_change_detected(self, device, job_in_progress, job_id, job_num_tasks, job_cur_task_id, job_cur_task_percentage):
         if job_id == "FilesystemMount" and self.is_device_inserted():
             print "Expected device %s inserted" % self._device
-            self.error = 0
-            self.loop.quit()
+            self._loop.quit()
 
     def add_detected(self, added_path):
         self._bus.add_signal_receiver(self.job_change_detected, signal_name='DeviceJobChanged', dbus_interface='org.freedesktop.UDisks')
@@ -27,14 +45,13 @@
     def remove_detected(self, removed_path):
         if not self.is_device_inserted():
             print "Expected device %s has been removed" % self._device
-            self.error = 0
-
-        self.loop.quit()
-    
+
+        self._loop.quit()
+
     def is_device_inserted(self):
         ud_manager_obj = self._bus.get_object("org.freedesktop.UDisks", "/org/freedesktop/UDisks")
         ud_manager = dbus.Interface(ud_manager_obj, 'org.freedesktop.UDisks')
-    
+
         for dev in ud_manager.EnumerateDevices():
             try:
                 device_obj = self._bus.get_object("org.freedesktop.UDisks", dev)
@@ -46,28 +63,17 @@
                 pass
 
         return False
-    
-    def __init__(self, device, action):
-        self._device = device
-        self._action = action
-        self._bus = dbus.SystemBus(mainloop=DBusGMainLoop())
-        self.loop = gobject.MainLoop()
-        self.error = 0
-        
-    	if self._action == 'insert':
-        	self._bus.add_signal_receiver(self.add_detected, signal_name='DeviceAdded', dbus_interface='org.freedesktop.UDisks')
-    	elif self._action == 'remove':
-        	self._bus.add_signal_receiver(self.remove_detected, signal_name='DeviceRemoved', dbus_interface='org.freedesktop.UDisks')
-
-        gobject.timeout_add_seconds(10, self.timeout)
-
-if __name__ == "__main__":
+
+def main():
     parser = ArgumentParser(description="Wait for the specified device to be inserted or removed.")
-    parser.add_argument('--device', required=True, choices=['usb','firewire','sdio'])
-    parser.add_argument('--action', required=True, choices=['insert','remove'])
+    parser.add_argument('action', choices=['insert','remove'])
+    parser.add_argument('device', choices=['usb','firewire','sdio'])
+    parser.add_argument('--timeout', type=int, default=10)
     args = parser.parse_args()
-        
-    listener = StorageDeviceListener(args.device, args.action)
-    listener.loop.run()
-    
-    sys.exit(listener.error)
+
+    listener = StorageDeviceListener(args.action, args.device)
+    return listener.check(args.timeout)
+
+
+if __name__ == "__main__":
+    sys.exit(main())