Merge lp:~yuningdodo/usb-creator/usb-creator.lp1413494-auto-mount-after-format into lp:usb-creator

Proposed by Yu Ning
Status: Rejected
Rejected by: Mathieu Trudel-Lapierre
Proposed branch: lp:~yuningdodo/usb-creator/usb-creator.lp1413494-auto-mount-after-format
Merge into: lp:usb-creator
Diff against target: 34 lines (+9/-1)
1 file modified
bin/usb-creator-helper (+9/-1)
To merge this branch: bzr merge lp:~yuningdodo/usb-creator/usb-creator.lp1413494-auto-mount-after-format
Reviewer Review Type Date Requested Status
Yu Ning (community) Disapprove
Mathieu Trudel-Lapierre Needs Fixing
Brian Murray Pending
Review via email: mp+250256@code.launchpad.net

Description of the change

I checked the source code and found usb-creator can only calculate the free space for a mounted partition, however the newly formated partition isn't mounted.

So to fix the issue we could mount it automatically after the formating. In the attached patch this job is done in the dbus method Format() in usb-creator-helper. In my own test it works fine.

To post a comment you must log in.
Revision history for this message
Yu Ning (yuningdodo) wrote :

One problem for this patch is it will mount the new partition as root, so the normal user can not unmount it.

I have proposed another patch to mount the new partition as the normal user:
https://code.launchpad.net/~yuningdodo/usb-creator/usb-creator.lp1413494v2-update-free-space-after-format/+merge/250258

Anyway, I think below change is still needed, although it's a different issue:

8 - unmount_all(udisks, dev)
9 + unmount_all(udisks, parent_dev)

Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

In this case, could you please either update this request appropriately to only keep that change, or provide it in a new merge proposal (along with a bug link if possible?)

Which bug would the umount_all change fix?

review: Needs Fixing
Revision history for this message
Yu Ning (yuningdodo) wrote :

Mathieu, thanks for the review. I checked the code again, looks like I had a misunderstanding about the umount_all() function, we shouldn't change the argument from dev to parent_dev. Please help to reject this merge proposal.

review: Disapprove

Unmerged revisions

463. By Yu Ning

* auto mount the new partition to calc free space correctly. (LP: #1413494)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bin/usb-creator-helper'
--- bin/usb-creator-helper 2014-09-02 20:12:12 +0000
+++ bin/usb-creator-helper 2015-02-19 05:17:59 +0000
@@ -231,7 +231,7 @@
231 check_system_internal(dev)231 check_system_internal(dev)
232232
233 # TODO LOCK233 # TODO LOCK
234 unmount_all(udisks, dev)234 unmount_all(udisks, parent_dev)
235 # Do NOT use the disk if asked to format a partition.235 # Do NOT use the disk if asked to format a partition.
236 # We still need to obtain the disk device name to zero out the MBR236 # We still need to obtain the disk device name to zero out the MBR
237 part = dev.get_partition()237 part = dev.get_partition()
@@ -243,6 +243,9 @@
243 part.call_set_type_sync('0x0c', no_options, None)243 part.call_set_type_sync('0x0c', no_options, None)
244 part.call_set_flags_sync(boot_dos_flag, no_options, None)244 part.call_set_flags_sync(boot_dos_flag, no_options, None)
245 block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)245 block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
246
247 # Mount the partition for the frontend to calc free space
248 dev.get_filesystem().call_mount_sync(no_options, None)
246 else:249 else:
247 # Create a new partition table and a FAT partition.250 # Create a new partition table and a FAT partition.
248 # Is this correct call to create partition table ?!251 # Is this correct call to create partition table ?!
@@ -262,6 +265,11 @@
262 block = obj.get_block()265 block = obj.get_block()
263 block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)266 block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
264267
268 # Mount the partition for the frontend to calc free space
269 udisks = UDisks.Client.new_sync(None)
270 obj = udisks.get_object(partition)
271 obj.get_filesystem().call_mount_sync(no_options, None)
272
265 # Zero out the MBR. Will require fancy privileges.273 # Zero out the MBR. Will require fancy privileges.
266 parent_block = parent_dev.get_block()274 parent_block = parent_dev.get_block()
267 parent_file = parent_block.get_cached_property('Device').get_bytestring().decode('utf-8')275 parent_file = parent_block.get_cached_property('Device').get_bytestring().decode('utf-8')

Subscribers

People subscribed via source and target branches

to all changes: