Handle the case where blkid(8) detects both a filesystem and a partition table

The F15 x86_64 live cd is both an ISO filesystem and a partition
table, presumably on purpose:

 # blkid -o udev -p ~davidz/Downloads/Fedora-15-x86_64-Live-Desktop.iso
 ID_FS_LABEL=Fedora-15-x86_64-Live-Desktop.is
 ID_FS_LABEL_ENC=Fedora-15-x86_64-Live-Desktop.is
 ID_FS_TYPE=iso9660
 ID_FS_USAGE=filesystem
 ID_PART_TABLE_TYPE=dos

If just used on an optical disc this presents no problem per se
because the kernel does not create partitions. However, if you dd the
image to a USB stick, the kernel will in fact recognize the partition
table and create a partition for it:

 # blkid -o udev -p /dev/sdb
 ID_FS_LABEL=Fedora-15-x86_64-Live-Desktop.is
 ID_FS_LABEL_ENC=Fedora-15-x86_64-Live-Desktop.is
 ID_FS_TYPE=iso9660
 ID_FS_USAGE=filesystem
 ID_PART_TABLE_TYPE=dos

 # blkid -o udev -p /dev/sdb1
 ID_FS_LABEL=Fedora-15-x86_64-Live-Desktop.is
 ID_FS_LABEL_ENC=Fedora-15-x86_64-Live-Desktop.is
 ID_FS_TYPE=iso9660
 ID_FS_USAGE=filesystem
 ID_PART_TABLE_TYPE=dos
 ID_PART_ENTRY_SCHEME=dos
 ID_PART_ENTRY_TYPE=0x17
 ID_PART_ENTRY_FLAGS=0x80
 ID_PART_ENTRY_NUMBER=1
 ID_PART_ENTRY_OFFSET=0
 ID_PART_ENTRY_SIZE=1161216
 ID_PART_ENTRY_DISK=8:16

There is nothing, per se, wrong with this ... except that applications
(such as udisks and GNOME sitting on top) need to be careful and not
e.g. offer *two icons* for the iso9660 filesystem (one for sdb and one
for sdb1).

The fix is fortunately simple - we only allow a 'disk' block device to
be a PartitionTable or a Filesystem and not both at the same time. The
way we do this is to look at whether the kernel has already
partitioned the device or not (and this works only because the kernel
partiitoner guarantees that partition kobjects are created prior to
announcing the 'disk' block device).

Also stress in the docs that applications shouldn't be looking at
IdUsage or IdType (a direct copy of whatever blkid(8) puts in the udev
database) and that they instead should be looking at what D-Bus
interfaces are implemented.

Signed-off-by: David Zeuthen <davidz@redhat.com>

diff --git a/data/org.freedesktop.UDisks2.xml b/data/org.freedesktop.UDisks2.xml
index 2b34f7e..6b96ae8 100644 (file)
--- a/data/org.freedesktop.UDisks2.xml
+++ b/data/org.freedesktop.UDisks2.xml
@@ -500,8 +500,17 @@
          necessarily mean the device contains no structured data; it
          only means that no signature known to the probing code was
          detected.
+
+         Applications should not rely on the value in this or the
+         #org.freedesktop.UDisks2.Block:IdType property - instead,
+         applications should check for whether the object in question
+         implements interfaces such as
+         e.g. #org.freedesktop.UDisks2.Filesystem,
+         #org.freedesktop.UDisks2.Swapspace or
+         #org.freedesktop.UDisks2.Encrypted.
     -->
     <property name="IdUsage" type="s" access="read"/>
+
     <!--
         IdType:
         This property contains more information about the result of
@@ -521,8 +530,10 @@
           <listitem><para>Something else. Known values include <literal>swap</literal> (for swap space), <literal>suspend</literal> (data used when resuming from suspend-to-disk.</para></listitem>
         </varlistentry>
         </variablelist>
+        See the note for the #org.freedesktop.UDisks2.Block:IdUsage property about usage.
     -->
     <property name="IdType" type="s" access="read"/>
+
     <!-- IdVersion:
          The version of the filesystem or other structured data on the block device.
          Do not make any assumptions about the format.

diff --git a/src/udiskslinuxblockobject.c b/src/udiskslinuxblockobject.c
index b786688..1c49ed1 100644 (file)
--- a/src/udiskslinuxblockobject.c
+++ b/src/udiskslinuxblockobject.c
@@ -410,16 +410,49 @@ block_device_update (UDisksLinuxBlockObject *object,
 }
 
 /* ---------------------------------------------------------------------------------------------------- */
-/* org.freedesktop.UDisks.PartitionTable */
 
 static gboolean
-partition_table_check (UDisksLinuxBlockObject *object)
+disk_is_partitioned_by_kernel (GUdevDevice *device)
 {
   gboolean ret = FALSE;
   GDir *dir = NULL;
   const gchar *name;
   const gchar *device_name;
 
+  g_return_val_if_fail (g_strcmp0 (g_udev_device_get_devtype (device), "disk") == 0, FALSE);
+
+  dir = g_dir_open (g_udev_device_get_sysfs_path (device), 0 /* flags */, NULL /* GError */);
+  if (dir == NULL)
+    goto out;
+
+  device_name = g_udev_device_get_name (device);
+  while ((name = g_dir_read_name (dir)) != NULL)
+    {
+      /* TODO: could check that it's a block device - for now, just
+       * checking the name suffices
+       */
+      if (g_str_has_prefix (name, device_name))
+        {
+          ret = TRUE;
+          goto out;
+        }
+    }
+
+ out:
+  if (dir != NULL)
+    g_dir_close (dir);
+  g_debug ("dipbk %s -> %d", g_udev_device_get_name (device), ret);
+  return ret;
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+/* org.freedesktop.UDisks.PartitionTable */
+
+static gboolean
+partition_table_check (UDisksLinuxBlockObject *object)
+{
+  gboolean ret = FALSE;
+
   /* only consider whole disks, never partitions */
   if (g_strcmp0 (g_udev_device_get_devtype (object->device), "disk") != 0)
     goto out;
@@ -427,6 +460,20 @@ partition_table_check (UDisksLinuxBlockObject *object)
   /* if blkid(8) already identified the device as a partition table, it's all good */
   if (g_udev_device_has_property (object->device, "ID_PART_TABLE_TYPE"))
     {
+      /* however, if blkid(8) also think that we're a filesystem... then don't
+       * mark us as a partition table ... except if we are partitioned by the
+       * kernel
+       *
+       * (see filesystem_check() for the similar case where we don't pretend
+       * to be a filesystem)
+       */
+      if (g_strcmp0 (g_udev_device_get_property (object->device, "ID_FS_USAGE"), "filesystem") == 0)
+        {
+          if (!disk_is_partitioned_by_kernel (object->device))
+            {
+              goto out;
+            }
+        }
       ret = TRUE;
       goto out;
     }
@@ -441,27 +488,13 @@ partition_table_check (UDisksLinuxBlockObject *object)
    * children... then it must be partitioned by the kernel, hence it
    * must contain a partition table.
    */
-  dir = g_dir_open (g_udev_device_get_sysfs_path (object->device), 0 /* flags */, NULL /* GError */);
-  if (dir == NULL)
-    goto out;
-
-  device_name = g_udev_device_get_name (object->device);
-  while ((name = g_dir_read_name (dir)) != NULL)
+  if (disk_is_partitioned_by_kernel (object->device))
     {
-      /* TODO: could check that it's a block device - for now, just
-       * checking the name suffices
-       */
-      if (g_str_has_prefix (name, device_name))
-        {
-          ret = TRUE;
-          goto out;
-        }
+      ret = TRUE;
+      goto out;
     }
 
  out:
-  if (dir != NULL)
-    g_dir_close (dir);
-
   return ret;
 }
 
@@ -543,12 +576,29 @@ drive_does_not_detect_media_change (UDisksLinuxBlockObject *object)
 static gboolean
 filesystem_check (UDisksLinuxBlockObject *object)
 {
-  gboolean ret;
+  gboolean ret = FALSE;
+  gboolean detected_as_filesystem = FALSE;
   UDisksMountType mount_type;
 
-  ret = FALSE;
+  /* if blkid(8) has detected the device as a filesystem, trust that */
+  if (g_strcmp0 (udisks_block_get_id_usage (object->iface_block_device), "filesystem") == 0)
+    {
+      detected_as_filesystem = TRUE;
+      /* except, if we are a whole-disk device and the kernel has already partitioned us...
+       * in that case, don't pretend we're a filesystem
+       *
+       * (see partition_table_check() above for the similar case where we don't pretend
+       * to be a partition table)
+       */
+      if (g_strcmp0 (g_udev_device_get_devtype (object->device), "disk") == 0 &&
+          disk_is_partitioned_by_kernel (object->device))
+        {
+          detected_as_filesystem = FALSE;
+        }
+    }
+
   if (drive_does_not_detect_media_change (object) ||
-      g_strcmp0 (udisks_block_get_id_usage (object->iface_block_device), "filesystem") == 0 ||
+      detected_as_filesystem ||
       (udisks_mount_monitor_is_dev_in_use (object->mount_monitor,
                                            g_udev_device_get_device_number (object->device),
                                            &mount_type) &&