Merge lp:~yuningdodo/usb-creator/usb-creator.lp1300361-format-parent-device-to-erase-correctly into lp:usb-creator

[Yu Ning]

* usb-creator-helper: Always format the parent device to fix the issue that
  usb stick fail to be erased if there is no partition table. (LP: #1300361)

To verify this patch, create an empty usb stick with dd:

    sudo dd if=/dev/zero of=/dev/sdb bs=1M count=1

Then erase it with usb-creator, the operation should complete successfully. However there is also bug #1318954 that would disturb the erase operation, on my own box I have to modify util-linux to ignore bug #1318954 to verify this patch.

[Dimitri John Ledkov (xnox)]

I thought that resolution of bug #1318954 would be sufficient.

From reading below, I'm not sure we are not going to regress the following past regression:
given a usb-stick with two partitions, formatting one of the partitions, should preserve all data on the other partition.

Will test that when I get my hands on a usb-stick tomorrow.

Thanks for this patch.

[Yu Ning (yuningdodo)]

Thanks for the comments. So if I understand correctly it's designed to format only the selected partition but not the whole disk? Personally I thought the "format the whole disk" is a useful feature to some people like me, could we add an GUI option to set what to format, the partition or the disk?

[Yu Ning (yuningdodo)]

This patch is no longer needed since the issue is already fixed.

review: Disapprove

Unmerged revisions

443. By Yu Ning on 2014-07-07

usb-creator-helper: Always format the parent device to fix the issue that
usb stick fail to be erased if there is no partition table. (LP: #1300361)

=== modified file 'bin/usb-creator-helper'
--- bin/usb-creator-helper	2014-06-16 09:26:40 +0000
+++ bin/usb-creator-helper	2014-07-07 10:12:25 +0000
@@ -43,11 +43,13 @@
 
 def _get_parent_object(udisks, device_name):
     obj = udisks.get_object(_get_object_path_from_device(device_name))
-    if obj.get_partition_table():
+    partition = obj.get_partition()
+    if not partition:
         return obj
-    partition = obj.get_partition()
     parent = partition.get_cached_property('Table').get_string()    
-    return udisks.get_object(parent)
+    if udisks.get_object(parent):
+        return udisks.get_object(parent)
+    return obj
     
 def unmount_all(udisks, parent):
     '''Unmounts the device or any partitions of the device.'''
@@ -214,8 +216,7 @@
         # TODO evand 2009-08-25: Needs a confirmation dialog.
         # XXX test with a device that doesn't have a partition table.
         udisks = UDisks.Client.new_sync(None)
-        dev = udisks.get_object(device)
-        parent_dev = _get_parent_object(udisks, device)
+        dev = _get_parent_object(udisks, device)
         
         if not allow_system_internal:
             check_system_internal(dev)
@@ -224,30 +225,29 @@
         unmount_all(udisks, dev)
         # Do NOT use the disk if asked to format a partition.
         # We still need to obtain the disk device name to zero out the MBR
-        part = dev.get_partition()
         block = dev.get_block()
-        if part:
-            # Create the partition
-            #type, label, flags
-            boot_dos_flag=64
-            part.call_set_type_sync('0x0c', no_options, None)
-            part.call_set_flags_sync(boot_dos_flag, no_options, None)
-            block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
-        else:
-            # Create a new partition table and a FAT partition.
-            # Is this correct call to create partition table ?!
-            block.call_format_sync('dos', GLib.Variant('a{sv}', {'erase': GLib.Variant('s', 'zero')}), None)
-            size = block.get_cached_property('Size').get_uint64()
-            table = dev.get_partition_table()
-            partition = table.call_create_partition_sync(0, size, '0x0c', '', no_options, None) 
-            obj = udisks.get_object(partition)
-            block = obj.get_block()
-            block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
+        device = block.get_cached_property('Device').get_bytestring().decode('utf-8')
+
+        # Create a new partition table and a FAT partition.
+        # Is this correct call to create partition table ?!
+        block.call_format_sync('dos', no_options, None)
+        size = block.get_cached_property('Size').get_uint64()
+
+        # FIXME: dev.get_partition_table() will return None unless we get dev
+        #        with a new client. Is there any better way for it?
+        udisks = UDisks.Client.new_sync(None)
+        dev = _get_parent_object(udisks, device)
+
+        table = dev.get_partition_table()
+        partition = table.call_create_partition_sync(0, size, '0x0c', '', no_options, None) 
+
+        udisks = UDisks.Client.new_sync(None)
+        obj = udisks.get_object(partition)
+        block = obj.get_block()
+        block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
 
         # Zero out the MBR.  Will require fancy privileges.
-        parent_block = parent_dev.get_block()
-        parent_file = parent_block.get_cached_property('Device').get_bytestring().decode('utf-8')
-        popen(['dd', 'if=/dev/zero', 'of=%s' % parent_file, 'bs=446', 'count=1'])
+        popen(['dd', 'if=/dev/zero', 'of=%s' % device, 'bs=446', 'count=1'])
         # TODO UNLOCK
 
     @dbus.service.method(USBCREATOR_IFACE, in_signature='ssb', out_signature='',

=== modified file 'debian/changelog'
--- debian/changelog	2014-06-16 09:26:56 +0000
+++ debian/changelog	2014-07-07 10:12:25 +0000
@@ -1,3 +1,10 @@
+usb-creator (0.2.59) UNRELEASED; urgency=medium
+
+  * usb-creator-helper: Always format the parent device to fix the issue that
+    usb stick fail to be erased if there is no partition table. (LP: #1300361)
+
+ -- Yu Ning <[e-mail]>  Mon, 07 Jul 2014 17:53:07 +0800
+
 usb-creator (0.2.58) utopic; urgency=medium
 
   * Use "zero" erase type instead of the invalid "". (LP: #1294877)