Merge lp:~salgado/linaro-image-tools/bug-673412 into lp:linaro-image-tools/11.11

[Guilherme Salgado]

Fix bug 673412 by ensuring boot/root filesystems are umounted before we attempt to mkfs them.

[James Westby (james-w)]

29 + if filesystem in open('/proc/mounts').read():

Is that a bit broad? It would cause an error if the filesystem showed
up in there in a strange place wouldn't it?

Thanks,

James

review: Needs Information

[Guilherme Salgado (salgado)]

On Wed, 2011-01-05 at 17:32 +0000, James Westby wrote:
> Review: Needs Information
> 29 + if filesystem in open('/proc/mounts').read():
>
> Is that a bit broad? It would cause an error if the filesystem showed
> up in there in a strange place wouldn't it?

I find it highly unlikely that a /dev/{sdXX,mmcblkNpN,loopN} string
(which is usually what the function will be given) ends up there unless
the filesystem is mounted, but I can use UDisks to do a more specific
check.

[lp:~salgado/linaro-image-tools/bug-673412]

232. By Guilherme Salgado on 2011-01-05

Use the UDisks dbus interface to check whether or not a given partition is mounted

[James Westby (james-w)]

On Wed, 05 Jan 2011 18:22:51 -0000, Guilherme Salgado <email address hidden> wrote:
> On Wed, 2011-01-05 at 17:32 +0000, James Westby wrote:
> > Review: Needs Information
> > 29 + if filesystem in open('/proc/mounts').read():
> >
> > Is that a bit broad? It would cause an error if the filesystem showed
> > up in there in a strange place wouldn't it?
>
> I find it highly unlikely that a /dev/{sdXX,mmcblkNpN,loopN} string
> (which is usually what the function will be given) ends up there unless
> the filesystem is mounted, but I can use UDisks to do a more specific
> check.

Yeah, it's rare. I'm happy with this going in as is and we can see if we
get any fallout.

Thanks,

James

[Guilherme Salgado (salgado)]

http://bazaar.launchpad.net/~salgado/linaro-image-tools/bug-673412/revision/232 has the changes to use the UDisks dbus interface instead of /proc/mounts

[James Westby (james-w)]

On Wed, 05 Jan 2011 19:37:52 -0000, Guilherme Salgado <email address hidden> wrote:
> http://bazaar.launchpad.net/~salgado/linaro-image-tools/bug-673412/revision/232
> has the changes to use the UDisks dbus interface instead of
> /proc/mounts

Oh, great. Looks good, thanks.

James

=== modified file 'media_create/partitions.py'
--- media_create/partitions.py	2011-01-04 14:27:26 +0000
+++ media_create/partitions.py	2011-01-05 19:35:20 +0000
@@ -15,6 +15,8 @@
 SECTORS = 63
 SECTOR_SIZE = 512 # bytes
 CYLINDER_SIZE = HEADS * SECTORS * SECTOR_SIZE
+DBUS_PROPERTIES = 'org.freedesktop.DBus.Properties'
+UDISKS = "org.freedesktop.UDisks"
 
 
 # I wonder if it'd make sense to convert this into a small shim which calls
@@ -56,6 +58,10 @@
 
     if should_format_bootfs:
         print "\nFormating boot partition\n"
+        # It looks like KDE somehow automounts the partitions after you
+        # repartition a disk so we need to unmount them here to create the
+        # filesystem.
+        ensure_partition_is_not_mounted(bootfs)
         proc = cmd_runner.run(
             ['mkfs.vfat', '-F', str(fat_size), bootfs, '-n', bootfs_label],
             as_root=True)
@@ -64,6 +70,7 @@
     if should_format_rootfs:
         print "\nFormating root partition\n"
         mkfs = 'mkfs.%s' % rootfs_type
+        ensure_partition_is_not_mounted(rootfs)
         proc = cmd_runner.run(
             [mkfs, '-U', rootfs_uuid, rootfs, '-L', rootfs_label],
             as_root=True)
@@ -72,6 +79,23 @@
     return bootfs, rootfs
 
 
+def ensure_partition_is_not_mounted(partition):
+    """Ensure the given partition is not mounted, umounting if necessary."""
+    if is_partition_mounted(partition):
+        cmd_runner.run(['umount', partition], as_root=True).wait()
+
+
+def is_partition_mounted(partition):
+    """Is the given partition mounted?"""
+    device_path = _get_udisks_device_path(partition)
+    device = dbus.SystemBus().get_object(UDISKS, device_path)
+    is_partition = device.Get(
+        device_path, 'DeviceIsPartition', dbus_interface=DBUS_PROPERTIES)
+    assert is_partition, "%s is not a partition" % partition
+    return device.Get(
+        device_path, 'DeviceIsMounted', dbus_interface=DBUS_PROPERTIES)
+
+
 def get_boot_and_root_loopback_devices(image_file):
     """Return the boot and root loopback devices for the given image file.
 
@@ -156,15 +180,18 @@
     """Return the number of partitions on the given media."""
     # We could do the same easily using python-parted but it requires root
     # rights to read block devices, so we use UDisks here.
+    device_path = _get_udisks_device_path(media.path)
+    device = dbus.SystemBus().get_object(UDISKS, device_path)
+    return device.Get(
+        device_path, 'PartitionTableCount', dbus_interface=DBUS_PROPERTIES)
+
+
+def _get_udisks_device_path(device):
+    """Return the UDisks path for the given device."""
     bus = dbus.SystemBus()
     udisks = dbus.Interface(
-        bus.get_object("org.freedesktop.UDisks", "/org/freedesktop/UDisks"),
-        'org.freedesktop.UDisks')
-    device_path = udisks.get_dbus_method('FindDeviceByDeviceFile')(media.path)
-    device = bus.get_object("org.freedesktop.UDisks", device_path)
-    return device.Get(
-        device_path, 'partition-table-count',
-        dbus_interface='org.freedesktop.DBus.Properties')
+        bus.get_object(UDISKS, "/org/freedesktop/UDisks"), UDISKS)
+    return udisks.get_dbus_method('FindDeviceByDeviceFile')(device)
 
 
 def convert_size_to_bytes(size):

=== modified file 'media_create/tests/test_media_create.py'
--- media_create/tests/test_media_create.py	2011-01-05 14:36:26 +0000
+++ media_create/tests/test_media_create.py	2011-01-05 19:35:20 +0000
@@ -29,6 +29,7 @@
     calculate_partition_size_and_offset,
     convert_size_to_bytes,
     create_partitions,
+    ensure_partition_is_not_mounted,
     get_boot_and_root_loopback_devices,
     get_boot_and_root_partitions_for_media,
     Media,
@@ -423,6 +424,21 @@
         self.assertIn('Successfully wrote the new partition table', stdout)
         return tempfile
 
+    def test_ensure_partition_is_not_mounted_for_mounted_partition(self):
+        self.useFixture(MockSomethingFixture(
+            partitions, 'is_partition_mounted', lambda part: True))
+        popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        ensure_partition_is_not_mounted('/dev/whatever')
+        self.assertEqual(
+            [['sudo', 'umount', '/dev/whatever']], popen_fixture.mock.calls)
+
+    def test_ensure_partition_is_not_mounted_for_umounted_partition(self):
+        self.useFixture(MockSomethingFixture(
+            partitions, 'is_partition_mounted', lambda part: False))
+        popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
+        ensure_partition_is_not_mounted('/dev/whatever')
+        self.assertEqual(None, popen_fixture.mock.calls)
+
     def test_get_boot_and_root_loopback_devices(self):
         tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
@@ -446,6 +462,11 @@
         popen_fixture = self.useFixture(MockCmdRunnerPopenFixture())
         self.useFixture(MockSomethingFixture(
             sys, 'stdout', open('/dev/null', 'w')))
+        self.useFixture(MockSomethingFixture(
+            partitions, 'is_partition_mounted', lambda part: False))
+        self.useFixture(MockSomethingFixture(
+            partitions, 'get_boot_and_root_loopback_devices',
+            lambda image: ('/dev/loop99', '/dev/loop98')))
         uuid = '2e82008e-1af3-4699-8521-3bf5bac1e67a'
         bootfs, rootfs = setup_partitions(
             'beagle', Media(tempfile), 32, '2G', 'boot', 'root', 'ext3',
@@ -458,12 +479,6 @@
               tempfile],
              # Make sure changes are written to disk.
              ['sync'],
-             # Register boot/root loopback devices so that we can just copy
-             # their contents over and have it written to the image file.
-             ['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
-              '32256', '--sizelimit', '129024'],
-             ['sudo', 'losetup', '-f', '--show', tempfile, '--offset',
-              '161280', '--sizelimit', '10321920'],
              ['sudo', 'mkfs.vfat', '-F', '32', bootfs, '-n', 'boot'],
              ['sudo', 'mkfs.ext3', '-U', uuid, rootfs, '-L', 'root']],
             popen_fixture.mock.calls)
@@ -473,6 +488,9 @@
             sys, 'stdout', open('/dev/null', 'w')))
         self.useFixture(MockSomethingFixture(
             partitions, '_get_partition_count', lambda media: 2))
+        # Pretend the partitions are mounted.
+        self.useFixture(MockSomethingFixture(
+            partitions, 'is_partition_mounted', lambda part: True))
         tempfile = self._create_qemu_img_with_partitions(',1,0x0C,*\n,,,-')
         media = Media(tempfile)
         # Pretend our tempfile is a block device.
@@ -486,7 +504,11 @@
             [['sudo', 'parted', '-s', tempfile, 'mklabel', 'msdos'],
              ['sudo', 'sfdisk', '-D', '-H', '255', '-S', '63', tempfile],
              ['sync'],
+             # Since the partitions are mounted, setup_partitions will umount
+             # them before running mkfs.
+             ['sudo', 'umount', bootfs],
              ['sudo', 'mkfs.vfat', '-F', '32', bootfs, '-n', 'boot'],
+             ['sudo', 'umount', rootfs],
              ['sudo', 'mkfs.ext3', '-U', uuid, rootfs, '-L', 'root']],
             popen_fixture.mock.calls)