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

Proposed by Yu Ning on 2015-02-19
Status: Rejected
Rejected by: Mathieu Trudel-Lapierre on 2015-03-05
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 on 2015-03-05
Mathieu Trudel-Lapierre (community) 2015-02-19 Needs Fixing on 2015-03-05
Brian Murray 2015-02-19 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.
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)

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
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 on 2015-02-19

* 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
1=== modified file 'bin/usb-creator-helper'
2--- bin/usb-creator-helper 2014-09-02 20:12:12 +0000
3+++ bin/usb-creator-helper 2015-02-19 05:17:59 +0000
4@@ -231,7 +231,7 @@
5 check_system_internal(dev)
6
7 # TODO LOCK
8- unmount_all(udisks, dev)
9+ unmount_all(udisks, parent_dev)
10 # Do NOT use the disk if asked to format a partition.
11 # We still need to obtain the disk device name to zero out the MBR
12 part = dev.get_partition()
13@@ -243,6 +243,9 @@
14 part.call_set_type_sync('0x0c', no_options, None)
15 part.call_set_flags_sync(boot_dos_flag, no_options, None)
16 block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
17+
18+ # Mount the partition for the frontend to calc free space
19+ dev.get_filesystem().call_mount_sync(no_options, None)
20 else:
21 # Create a new partition table and a FAT partition.
22 # Is this correct call to create partition table ?!
23@@ -262,6 +265,11 @@
24 block = obj.get_block()
25 block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
26
27+ # Mount the partition for the frontend to calc free space
28+ udisks = UDisks.Client.new_sync(None)
29+ obj = udisks.get_object(partition)
30+ obj.get_filesystem().call_mount_sync(no_options, None)
31+
32 # Zero out the MBR. Will require fancy privileges.
33 parent_block = parent_dev.get_block()
34 parent_file = parent_block.get_cached_property('Device').get_bytestring().decode('utf-8')

Subscribers

People subscribed via source and target branches

to all changes: