Re: [RFC] [PATCH] Generate stable device names in device.map on Linux

[Colin Watson]

On Fri, Jun 25, 2010 at 08:46:48PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko 
wrote:
> Colin Watson wrote:
> > === modified file 'util/deviceiter.c'
> > --- util/deviceiter.c       2010-06-11 20:31:16 +0000
> > +++ util/deviceiter.c       2010-06-21 08:54:07 +0000
> > @@ -345,18 +346,37 @@ get_xvd_disk_name (char *name, int unit)
> >  }
> >  #endif
> >  
> > +static struct seen_device
> > +{
> > +  struct seen_device *next;
> > +  const char *name;
> > +} *seen;
> > +
> >  /* Check if DEVICE can be read. If an error occurs, return zero,
> >     otherwise return non-zero.  */
> >  static int
> >  check_device (const char *device)
> >  {
> 
> This patch subtly changes the semantics of this function. It's a static
> one so the ramifications are relatively small. But you need at very
> least to resync the comment and changing name when changing semantics is
> recommended.

Fair enough.

> > +   names = xmalloc (names_max * sizeof *names);
> > +
> 
> Please use parentheses for sizeof

OK.

My personal practice is 'sizeof (TYPE)' but 'sizeof EXPR', to make the
distinction between the two more visible, and my opinion is that this is
implied by the C standard.  Indeed, the GNU Coding Standards include an
example of the form 'sizeof EXPR':

  
http://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#Syntactic-Conventions

I'm happy to vary that for GRUB if you prefer, though.

> > +   qsort (names, names_len, sizeof *names, &compare_file_names);
> > +
> 
> Which part of code uses that array is sorted?

I've added a comment.  Please see the following revised patch.

2010-06-21  Colin Watson  <address@hidden>

        Change grub-mkdevicemap to emit /dev/disk/by-id/ names where
        possible on Linux.

        * util/deviceiter.c (check_device): Rename to ...
        (check_device_readable_unique): ... this.  Update all callers.
        Maintain and check a list of which devices (by canonicalized name)
        have already been seen.
        (clear_seen_devices): New function.
        (compare_file_names) [__linux__]: New function.
        (grub_util_iterate_devices): Clear the list of seen devices on exit
        and (just in case) on entry.
        (grub_util_iterate_devices) [__linux__]: Iterate over non-partition
        devices in /dev/disk/by-id/, in sorted order.  Remove DM-RAID
        seen-devices list, superseded by general code in check_device.

=== modified file 'util/deviceiter.c'
--- util/deviceiter.c   2010-06-11 20:31:16 +0000
+++ util/deviceiter.c   2010-06-26 10:09:53 +0000
@@ -28,6 +28,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
+#include <dirent.h>
 
 #include <grub/util/misc.h>
 #include <grub/util/deviceiter.h>
@@ -345,18 +346,37 @@ get_xvd_disk_name (char *name, int unit)
 }
 #endif
 
-/* Check if DEVICE can be read. If an error occurs, return zero,
-   otherwise return non-zero.  */
+static struct seen_device
+{
+  struct seen_device *next;
+  const char *name;
+} *seen;
+
+/* Check if DEVICE can be read.  Skip any DEVICE that we have already seen.
+   If an error occurs, return zero, otherwise return non-zero.  */
 static int
-check_device (const char *device)
+check_device_readable_unique (const char *device)
 {
+  char *real_device;
   char buf[512];
   FILE *fp;
+  struct seen_device *seen_elt;
 
   /* If DEVICE is empty, just return error.  */
   if (*device == 0)
     return 0;
 
+  /* Have we seen this device already?  */
+  real_device = canonicalize_file_name (device);
+  if (! real_device)
+    return 0;
+  if (grub_named_list_find (GRUB_AS_NAMED_LIST (seen), real_device))
+    {
+      grub_dprintf ("deviceiter", "Already seen %s (%s)\n",
+                   device, real_device);
+      goto fail;
+    }
+
   fp = fopen (device, "r");
   if (! fp)
     {
@@ -379,7 +399,7 @@ check_device (const char *device)
          break;
        }
       /* Error opening the device.  */
-      return 0;
+      goto fail;
     }
 
   /* Make sure CD-ROMs don't get assigned a BIOS disk number
@@ -387,7 +407,7 @@ check_device (const char *device)
 #ifdef __linux__
 # ifdef CDROM_GET_CAPABILITY
   if (ioctl (fileno (fp), CDROM_GET_CAPABILITY, 0) >= 0)
-    return 0;
+    goto fail;
 # else /* ! CDROM_GET_CAPABILITY */
   /* Check if DEVICE is a CD-ROM drive by the HDIO_GETGEO ioctl.  */
   {
@@ -395,14 +415,14 @@ check_device (const char *device)
     struct stat st;
 
     if (fstat (fileno (fp), &st))
-      return 0;
+      goto fail;
 
     /* If it is a block device and isn't a floppy, check if HDIO_GETGEO
        succeeds.  */
     if (S_ISBLK (st.st_mode)
        && MAJOR (st.st_rdev) != FLOPPY_MAJOR
        && ioctl (fileno (fp), HDIO_GETGEO, &hdg))
-      return 0;
+      goto fail;
   }
 # endif /* ! CDROM_GET_CAPABILITY */
 #endif /* __linux__ */
@@ -410,7 +430,7 @@ check_device (const char *device)
 #if defined(__FreeBSD_kernel__) || defined(__NetBSD__) || defined(__OpenBSD__)
 # ifdef CDIOCCLRDEBUG
   if (ioctl (fileno (fp), CDIOCCLRDEBUG, 0) >= 0)
-    return 0;
+    goto fail;
 # endif /* CDIOCCLRDEBUG */
 #endif /* __FreeBSD_kernel__ || __NetBSD__ || __OpenBSD__ */
 
@@ -418,21 +438,43 @@ check_device (const char *device)
   if (fread (buf, 1, 512, fp) != 512)
     {
       fclose (fp);
-      return 0;
+      goto fail;
     }
 
+  /* Remember that we've seen this device.  */
+  seen_elt = xmalloc (sizeof (*seen_elt));
+  seen_elt->name = real_device; /* steal memory */
+  grub_list_push (GRUB_AS_LIST_P (&seen), GRUB_AS_LIST (seen_elt));
+
   fclose (fp);
   return 1;
+
+fail:
+  free (real_device);
+  return 0;
+}
+
+static void
+clear_seen_devices (void)
+{
+  while (seen)
+    {
+      struct seen_device *seen_elt = seen;
+      seen = seen->next;
+      free (seen_elt);
+    }
+  seen = NULL;
 }
 
 #ifdef __linux__
-# ifdef HAVE_DEVICE_MAPPER
-struct dmraid_seen
+/* Like strcmp, but doesn't require a cast for use as a qsort comparator.  */
+static int
+compare_file_names (const void *a, const void *b)
 {
-  struct dmraid_seen *next;
-  const char *name;
-};
-# endif /* HAVE_DEVICE_MAPPER */
+  const char *left = *(const char **) a;
+  const char *right = *(const char **) b;
+  return strcmp (left, right);
+}
 #endif /* __linux__ */
 
 void
@@ -441,6 +483,8 @@ grub_util_iterate_devices (int NESTED_FU
 {
   int i;
 
+  clear_seen_devices ();
+
   /* Floppies.  */
   for (i = 0; i < floppy_disks; i++)
     {
@@ -450,13 +494,63 @@ grub_util_iterate_devices (int NESTED_FU
       get_floppy_disk_name (name, i);
       if (stat (name, &st) < 0)
        break;
-      /* In floppies, write the map, whether check_device succeeds
-        or not, because the user just may not insert floppies.  */
+      /* In floppies, write the map, whether check_device_readable_unique
+         succeeds or not, because the user just may not insert floppies.  */
       if (hook (name, 1))
-       return;
+       goto out;
     }
 
 #ifdef __linux__
+  {
+    DIR *dir = opendir ("/dev/disk/by-id");
+
+    if (dir)
+      {
+       struct dirent *entry;
+       char **names;
+       size_t names_len = 0, names_max = 1024, i;
+
+       names = xmalloc (names_max * sizeof (*names));
+
+       /* Dump all the directory entries into names, resizing if
+          necessary.  */
+       for (entry = readdir (dir); entry; entry = readdir (dir))
+         {
+           /* Skip partition entries.  */
+           if (strstr (entry->d_name, "-part"))
+             continue;
+           if (names_len >= names_max)
+             {
+               names_max *= 2;
+               names = xrealloc (names, names_max * sizeof (*names));
+             }
+           names[names_len++] = xasprintf (entry->d_name);
+         }
+
+       /* /dev/disk/by-id/ usually has a few alternative identifications of
+          devices (e.g. ATA vs. SATA).  check_device_readable_unique will
+          ensure that we only get one for any given disk, but sort the list
+          so that the choice of which one we get is stable.  */
+       qsort (names, names_len, sizeof (*names), &compare_file_names);
+
+       closedir (dir);
+
+       /* Now add all the devices in sorted order.  */
+       for (i = 0; i < names_len; ++i)
+         {
+           char *path = xasprintf ("/dev/disk/by-id/%s", names[i]);
+           if (check_device_readable_unique (path))
+             {
+               if (hook (path, 0))
+                 goto out;
+             }
+           free (path);
+           free (names[i]);
+         }
+       free (names);
+      }
+  }
+
   if (have_devfs ())
     {
       i = 0;
@@ -476,10 +570,10 @@ grub_util_iterate_devices (int NESTED_FU
            {
              strcat (name, "/disc");
              if (hook (name, 0))
-               return;
+               goto out;
            }
        }
-      return;
+      goto out;
     }
 #endif /* __linux__ */
 
@@ -489,10 +583,10 @@ grub_util_iterate_devices (int NESTED_FU
       char name[16];
 
       get_ide_disk_name (name, i);
-      if (check_device (name))
+      if (check_device_readable_unique (name))
        {
          if (hook (name, 0))
-           return;
+           goto out;
        }
     }
 
@@ -503,10 +597,10 @@ grub_util_iterate_devices (int NESTED_FU
       char name[16];
 
       get_virtio_disk_name (name, i);
-      if (check_device (name))
+      if (check_device_readable_unique (name))
        {
          if (hook (name, 0))
-           return;
+           goto out;
        }
     }
 
@@ -516,10 +610,10 @@ grub_util_iterate_devices (int NESTED_FU
       char name[20];
 
       get_ataraid_disk_name (name, i);
-      if (check_device (name))
+      if (check_device_readable_unique (name))
        {
          if (hook (name, 0))
-           return;
+           goto out;
         }
     }
 
@@ -529,10 +623,10 @@ grub_util_iterate_devices (int NESTED_FU
       char name[16];
 
       get_xvd_disk_name (name, i);
-      if (check_device (name))
+      if (check_device_readable_unique (name))
        {
          if (hook (name, 0))
-           return;
+           goto out;
        }
     }
 #endif /* __linux__ */
@@ -543,10 +637,10 @@ grub_util_iterate_devices (int NESTED_FU
       char name[16];
 
       get_scsi_disk_name (name, i);
-      if (check_device (name))
+      if (check_device_readable_unique (name))
        {
          if (hook (name, 0))
-           return;
+           goto out;
        }
     }
 
@@ -566,10 +660,10 @@ grub_util_iterate_devices (int NESTED_FU
            char name[24];
 
            get_dac960_disk_name (name, controller, drive);
-           if (check_device (name))
+           if (check_device_readable_unique (name))
              {
                if (hook (name, 0))
-                 return;
+                 goto out;
              }
          }
       }
@@ -587,10 +681,10 @@ grub_util_iterate_devices (int NESTED_FU
            char name[24];
 
            get_acceleraid_disk_name (name, controller, drive);
-           if (check_device (name))
+           if (check_device_readable_unique (name))
              {
                if (hook (name, 0))
-                 return;
+                 goto out;
              }
          }
       }
@@ -608,10 +702,10 @@ grub_util_iterate_devices (int NESTED_FU
            char name[24];
 
            get_cciss_disk_name (name, controller, drive);
-           if (check_device (name))
+           if (check_device_readable_unique (name))
              {
                if (hook (name, 0))
-                 return;
+                 goto out;
              }
          }
       }
@@ -629,10 +723,10 @@ grub_util_iterate_devices (int NESTED_FU
            char name[24];
 
            get_ida_disk_name (name, controller, drive);
-           if (check_device (name))
+           if (check_device_readable_unique (name))
              {
                if (hook (name, 0))
-                 return;
+                 goto out;
              }
          }
       }
@@ -647,10 +741,10 @@ grub_util_iterate_devices (int NESTED_FU
        char name[24];
 
        get_i2o_disk_name (name, unit);
-       if (check_device (name))
+       if (check_device_readable_unique (name))
          {
            if (hook (name, 0))
-             return;
+             goto out;
          }
       }
   }
@@ -661,10 +755,10 @@ grub_util_iterate_devices (int NESTED_FU
       char name[16];
 
       get_mmc_disk_name (name, i);
-      if (check_device (name))
+      if (check_device_readable_unique (name))
        {
          if (hook (name, 0))
-           return;
+           goto out;
        }
     }
 
@@ -685,7 +779,6 @@ grub_util_iterate_devices (int NESTED_FU
       unsigned int next = 0;
       void *top_handle, *second_handle;
       struct dm_tree_node *root, *top, *second;
-      struct dmraid_seen *seen = NULL;
 
       /* Build DM tree for all devices.  */
       tree = dm_tree_create ();
@@ -721,7 +814,6 @@ grub_util_iterate_devices (int NESTED_FU
            {
              const char *node_name, *node_uuid;
              char *name;
-             struct dmraid_seen *seen_elt;
 
              node_name = dm_tree_node_get_name (second);
              dmraid_check (node_name, "dm_tree_node_get_name failed\n");
@@ -733,40 +825,21 @@ grub_util_iterate_devices (int NESTED_FU
                  goto dmraid_next_child;
                }
 
-             /* Have we already seen this node?  There are typically very few
-                DM-RAID disks, so a list should be fast enough.  */
-             if (grub_named_list_find (GRUB_AS_NAMED_LIST (seen), node_name))
-               {
-                 grub_dprintf ("deviceiter", "Already seen DM device %s\n",
-                               node_name);
-                 goto dmraid_next_child;
-               }
-
              name = xasprintf ("/dev/mapper/%s", node_name);
-             if (check_device (name))
+             if (check_device_readable_unique (name))
                {
                  if (hook (name, 0))
                    {
                      free (name);
-                     while (seen)
-                       {
-                         struct dmraid_seen *seen_elt = seen;
-                         seen = seen->next;
-                         free (seen_elt);
-                       }
                      if (task)
                        dm_task_destroy (task);
                      if (tree)
                        dm_tree_free (tree);
-                     return;
+                     goto out;
                    }
                }
              free (name);
 
-             seen_elt = xmalloc (sizeof *seen_elt);
-             seen_elt->name = node_name;
-             grub_list_push (GRUB_AS_LIST_P (&seen), GRUB_AS_LIST (seen_elt));
-
 dmraid_next_child:
              second = dm_tree_next_child (&second_handle, top, 1);
            }
@@ -774,12 +847,6 @@ dmraid_next_child:
        }
 
 dmraid_end:
-      while (seen)
-       {
-         struct dmraid_seen *seen_elt = seen;
-         seen = seen->next;
-         free (seen_elt);
-       }
       if (task)
        dm_task_destroy (task);
       if (tree)
@@ -787,5 +854,8 @@ dmraid_end:
     }
 # endif /* HAVE_DEVICE_MAPPER */
 #endif /* __linux__ */
+
+out:
+  clear_seen_devices ();
 }
 

Thanks,

-- 
Colin Watson                                       address@hidden