Merge lp:~yuningdodo/usb-creator/usb-creator.lp1424915-check-for-iso9660 into lp:usb-creator

Proposed by Yu Ning on 2015-02-24
Status: Needs review
Proposed branch: lp:~yuningdodo/usb-creator/usb-creator.lp1424915-check-for-iso9660
Merge into: lp:usb-creator
Diff against target: 80 lines (+34/-9)
2 files modified
bin/usb-creator-helper (+32/-8)
usbcreator/backends/udisks/backend.py (+2/-1)
To merge this branch: bzr merge lp:~yuningdodo/usb-creator/usb-creator.lp1424915-check-for-iso9660
Reviewer Review Type Date Requested Status
Mathieu Trudel-Lapierre 2015-02-24 Needs Fixing on 2015-03-06
Review via email: mp+250718@code.launchpad.net

Description of the change

To format an usbstick, which was previously dd'ed with an isohybrid ISO image, we need to format the whole drive instead of any partition. (LP: #1424915)

To post a comment you must log in.
Yu Ning (yuningdodo) wrote :

If we attempt to print the disk layout of an isohybrid ISO image with parted, we'll get below error messages:

$ file ubuntu-14.04.1-desktop-amd64.iso
ubuntu-14.04.1-desktop-amd64.iso: x86 boot sector

$ parted --script -- ubuntu-14.04.1-desktop-amd64.iso print
Warning: /home/yun/Downloads/iso/ubuntu-14.04.1-desktop-amd64.iso contains GPT signatures, indicating that it has a GPT table. However, it does not have a valid fake msdos partition table, as it should. Perhaps it was corrupted -- possibly by a program that doesn't understand GPT partition tables. Or perhaps you deleted the GPT table, and are now using an msdos partition table. Is this a GPT partition table?
Error: Both the primary and backup GPT tables are corrupt. Try making a fresh table, and using Parted's rescue feature to recover partitions.

We'll get the same error from an usbstick if it's dd'ed with the ISO.

In such a case most operations will fail or result in incorrect result, that's why we get the error messages in bug #1424915.

To fix this issue we should check if the usbstick is dd'ed with isohybrid ISO images. One possible way is checking for the drive's partition type since the isohybrid ISO image has type iso9660 for the drive.

In my own test this patch works on utopic and vivid. For trusty we also need to do some extra tricks due to the util-linux/wipefs bugs, we can discuss it later.

Mathieu Trudel-Lapierre (cyphermox) wrote :

I can't confirm this patch fixes the bug on vivid. Have you tested this specifically on vivid?

review: Needs Information
Yu Ning (yuningdodo) wrote :

No, I haven't tested on vivid yet, I'll give it a try.

464. By Yu Ning on 2015-03-06

Add delay and retry after each formating operation for udisks objects to be ready.

Yu Ning (yuningdodo) wrote :

Checked on vivid, the patch doesn't work unless we retry with some delay for udisks objects to be ready.

The branch has been updated, in my tests it can fix the bug on the daily vivid image (20150306).

Mathieu Trudel-Lapierre (cyphermox) wrote :

I'm not overly happy with sleeping to wait for objects, especially since you're using synchronous calls anyway which should be returning proper objects when they are called. The problem is that while delays can work in some circumstances, they tend to introduce issues on slower or differently-configured systems where the delay might not be sufficient.

I see in the API doc for udisks that there is a udisks_client_settle(UdisksClient *client) function, so it's possible that calling udisks.settle() after the get_object call might be sufficient to ensure we always get a valid partition table.

One further issue I see is that you're re-creating the udisks client in every loop iteration, something which shouldn't be necessary.

review: Needs Fixing
Yu Ning (yuningdodo) wrote :

Actually I have the same concern with you, sleep is never the preferred
solution to me. Thanks for the hint, I will give it a try next week.
2015年3月7日 上午1:38于 "Mathieu Trudel-Lapierre" <email address hidden>写道:

> Review: Needs Fixing
>
> I'm not overly happy with sleeping to wait for objects, especially since
> you're using synchronous calls anyway which should be returning proper
> objects when they are called. The problem is that while delays can work in
> some circumstances, they tend to introduce issues on slower or
> differently-configured systems where the delay might not be sufficient.
>
> I see in the API doc for udisks that there is a
> udisks_client_settle(UdisksClient *client) function, so it's possible that
> calling udisks.settle() after the get_object call might be sufficient to
> ensure we always get a valid partition table.
>
> One further issue I see is that you're re-creating the udisks client in
> every loop iteration, something which shouldn't be necessary.
> --
>
> https://code.launchpad.net/~yuningdodo/usb-creator/usb-creator.lp1424915-check-for-iso9660/+merge/250718
> You are the owner of
> lp:~yuningdodo/usb-creator/usb-creator.lp1424915-check-for-iso9660.
>

Yu Ning (yuningdodo) wrote :

I have made some tests with udisks_client_settle(), but no luck, we still can't get the new-create objects successfully.

Unmerged revisions

464. By Yu Ning on 2015-03-06

Add delay and retry after each formating operation for udisks objects to be ready.

463. By Yu Ning on 2015-02-24

To format an usbstick, which was previously dd'ed with an isohybrid ISO image,
we need to format the whole drive instead of any partition. (LP: #1424915)

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-03-06 09:35:23 +0000
4@@ -19,6 +19,7 @@
5 import dbus.service
6 import logging
7 import os
8+import time
9 logging.basicConfig(level=logging.DEBUG)
10
11 from dbus.mainloop.glib import DBusGMainLoop
12@@ -226,6 +227,14 @@
13 udisks = UDisks.Client.new_sync(None)
14 dev = udisks.get_object(device)
15 parent_dev = _get_parent_object(udisks, device)
16+ # Check if parent has the type iso9660, in such a case we should
17+ # format the whole disk instead of the partition.
18+ if parent_dev:
19+ parent_block = parent_dev.get_block()
20+ parent_id_type = parent_block.get_cached_property('IdType').get_string()
21+ if parent_id_type == 'iso9660':
22+ device = parent_dev.get_object_path()
23+ dev = udisks.get_object(device)
24
25 if not allow_system_internal:
26 check_system_internal(dev)
27@@ -251,16 +260,31 @@
28 size = block.get_cached_property('Size').get_uint64()
29 # unless we re-create the udisks client it will not return the
30 # device we created just now.
31- udisks = UDisks.Client.new_sync(None)
32- dev = udisks.get_object(device)
33- table = dev.get_partition_table()
34- partition = table.call_create_partition_sync(0, size, '0x0c', '', no_options, None)
35+ for i in range(10, 0, -1):
36+ udisks = UDisks.Client.new_sync(None)
37+ dev = udisks.get_object(device)
38+ table = dev.get_partition_table()
39+ if table:
40+ partition = table.call_create_partition_sync(0, size, '0x0c', '', no_options, None)
41+ if not partition:
42+ raise dbus.DBusException('com.ubuntu.USBCreator.Error.SystemInternal')
43+ break
44+ time.sleep(.1)
45+ if i == 9:
46+ raise dbus.DBusException('com.ubuntu.USBCreator.Error.SystemInternal')
47 # unless we re-create the udisks client it will not return the
48 # partition just now
49- udisks = UDisks.Client.new_sync(None)
50- obj = udisks.get_object(partition)
51- block = obj.get_block()
52- block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
53+ for i in range(10, 0, -1):
54+ udisks = UDisks.Client.new_sync(None)
55+ obj = udisks.get_object(partition)
56+ if obj:
57+ block = obj.get_block()
58+ if block:
59+ block.call_format_sync('vfat', GLib.Variant('a{sv}', {'label': GLib.Variant('s', '')}), None)
60+ break
61+ time.sleep(.1)
62+ if i == 9:
63+ raise dbus.DBusException('com.ubuntu.USBCreator.Error.SystemInternal')
64
65 # Zero out the MBR. Will require fancy privileges.
66 parent_block = parent_dev.get_block()
67
68=== modified file 'usbcreator/backends/udisks/backend.py'
69--- usbcreator/backends/udisks/backend.py 2015-01-17 00:03:17 +0000
70+++ usbcreator/backends/udisks/backend.py 2015-03-06 09:35:23 +0000
71@@ -294,7 +294,8 @@
72 def format_ended(self, dev=None):
73 self.format_done(dev)
74 obj = self.udisks.get_object(dev)
75- self._device_changed(obj)
76+ if obj:
77+ self._device_changed(obj)
78 self.format_ended_cb()
79
80 def format_failed(self, message, dev=None):

Subscribers

People subscribed via source and target branches

to all changes: