Re: Bug#554790: This breaks device.map on upgrade

[Colin Watson]

[Re-sending with full quoting and from my @ubuntu.com account so that it
doesn't get stuck in the grub-devel moderation queue.]

On Mon, Jul 12, 2010 at 12:26:21AM +0100, Colin Watson wrote:
> On Fri, Jul 09, 2010 at 08:01:12AM +0400, Vadim Solomin wrote:
> > This fix, at least in its current form, has a potential to break grub for 
> > users having more than one drive, unless they are careful enough to check 
> > and 
> > fix their device.map after upgrade.
> > 
> > Old mkdevicemaps assigned grub device numbers in the sort order of kernel 
> > device names, which was right more often than not. On the other hand, the 
> > sort 
> > order of (pretty much random) stable names, used by new version, is 
> > extremely 
> > unlikely to have any correlation to grub device order.
> > 
> > Included is a rough patch which preserves the kernel-name order for grub 
> > devices when generating device.map.
> 
> I like this idea; it seems that it ought to minimise the likelihood of
> upheaval due to the change in device.map generation.  I agree that
> nothing is particularly guaranteed here but it would be nice to try to
> minimise the chances of problems, if only to try to reduce the number of
> people who find they need to ask for help on #grub ...
> 
> Vladimir, would this patch need a copyright assignment?  Most of it
> seems pretty straightforward; in fact I doubt that it would come out
> very much different if I'd written it from scratch.

I've made a number of changes to this patch to fix up formatting and to
behave a bit closer to the way I expect; in particular it's necessary to
compare by ->stable if comparing by ->kernel returns zero, for the
reasons previously explained in a comment.

Vadim's original patch is quoted here, followed by my amended version
with a ChangeLog entry.

> > --- grub2-1.98+20100706/util/deviceiter.c   2010-07-06 20:57:30.000000000 
> > +0400
> > +++ grub2-1.98+20100706-new/util/deviceiter.c       2010-07-09 
> > 07:33:16.135823063 +0400
> > @@ -467,14 +467,21 @@
> >  }
> >  
> >  #ifdef __linux__
> > +struct device
> > +{
> > +   char *stable;
> > +   char *kernel;
> > +};
> > +
> >  /* 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)
> >  {
> > -  const char *left = *(const char **) a;
> > -  const char *right = *(const char **) b;
> > +  const char *left = ((const struct device *) a) -> kernel;
> > +  const char *right = ((const struct device *) b) -> kernel;
> >    return strcmp (left, right);
> >  }
> > +
> >  #endif /* __linux__ */
> >  
> >  void
> > @@ -507,10 +514,11 @@
> >      if (dir)
> >        {
> >     struct dirent *entry;
> > -   char **names;
> > -   size_t names_len = 0, names_max = 1024, i;
> > +   struct device *devs;
> > +   size_t devs_len = 0, devs_max = 1024, i;
> > +   char *path = 0;
> >  
> > -   names = xmalloc (names_max * sizeof (*names));
> > +   devs = xmalloc (devs_max * sizeof (*devs));
> >  
> >     /* Dump all the directory entries into names, resizing if
> >        necessary.  */
> > @@ -526,35 +534,39 @@
> >         /* Skip RAID entries; they are handled by upper layers.  */
> >         if (strncmp (entry->d_name, "md-", sizeof ("md-") - 1) == 0)
> >           continue;
> > -       if (names_len >= names_max)
> > +       if (devs_len >= devs_max)
> >           {
> > -           names_max *= 2;
> > -           names = xrealloc (names, names_max * sizeof (*names));
> > +           devs_max *= 2;
> > +           devs = xrealloc (devs, devs_max * sizeof (*devs));
> >           }
> > -       names[names_len++] = xasprintf (entry->d_name);
> > +       devs[devs_len].stable = xasprintf (entry->d_name);
> > +       path = xasprintf("/dev/disk/by-id/%s", entry->d_name);
> > +       devs[devs_len++].kernel = canonicalize_file_name(path);
> > +       free(path);
> >       }
> >  
> >     /* /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);
> > +   qsort (devs, devs_len, sizeof (*devs), &compare_file_names);
> >  
> >     closedir (dir);
> >  
> >     /* Now add all the devices in sorted order.  */
> > -   for (i = 0; i < names_len; ++i)
> > +   for (i = 0; i < devs_len; ++i)
> >       {
> > -       char *path = xasprintf ("/dev/disk/by-id/%s", names[i]);
> > +       char *path = xasprintf ("/dev/disk/by-id/%s", devs[i].stable);
> >         if (check_device_readable_unique (path))
> >           {
> >             if (hook (path, 0))
> >               goto out;
> >           }
> >         free (path);
> > -       free (names[i]);
> > +       free (devs[i].stable);
> > +       free (devs[i].kernel);
> >       }
> > -   free (names);
> > +   free (devs);
> >        }
> >    }
> >  

2010-07-12  Vadim Solomin  <address@hidden>
2010-07-12  Colin Watson  <address@hidden>

        Generate device.map in something closer to the old ordering.

        * util/deviceiter.c (struct device): New declaration.
        (compare_file_names): Rename to ...
        (compare_devices): ... this.  Sort by kernel name in preference
        to the stable by-id name, but keep the latter as a fallback
        comparison.  Update header comment.
        (grub_util_iterate_devices) [__linux__]: Construct and sort an
        array of `struct device' rather than of plain file names.

=== modified file 'util/deviceiter.c'
--- util/deviceiter.c   2010-07-06 14:10:36 +0000
+++ util/deviceiter.c   2010-07-12 10:34:16 +0000
@@ -467,13 +467,30 @@ clear_seen_devices (void)
 }
 
 #ifdef __linux__
-/* Like strcmp, but doesn't require a cast for use as a qsort comparator.  */
+struct device
+{
+       char *stable;
+       char *kernel;
+};
+
+/* Sort by the kernel name for preference since that most closely matches
+   older device.map files, but sort by stable by-id names as a fallback.
+   This is because /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.  */
 static int
-compare_file_names (const void *a, const void *b)
+compare_devices (const void *a, const void *b)
 {
-  const char *left = *(const char **) a;
-  const char *right = *(const char **) b;
-  return strcmp (left, right);
+  const struct device *left = (const struct device *) a;
+  const struct device *right = (const struct device *) b;
+  int ret;
+  ret = strcmp (left->kernel, right->kernel);
+  if (ret)
+    return ret;
+  else
+    return strcmp (left->stable, right->stable);
 }
 #endif /* __linux__ */
 
@@ -507,10 +524,10 @@ grub_util_iterate_devices (int NESTED_FU
     if (dir)
       {
        struct dirent *entry;
-       char **names;
-       size_t names_len = 0, names_max = 1024, i;
+       struct device *devs;
+       size_t devs_len = 0, devs_max = 1024, i;
 
-       names = xmalloc (names_max * sizeof (*names));
+       devs = xmalloc (devs_max * sizeof (*devs));
 
        /* Dump all the directory entries into names, resizing if
           necessary.  */
@@ -526,35 +543,34 @@ grub_util_iterate_devices (int NESTED_FU
            /* Skip RAID entries; they are handled by upper layers.  */
            if (strncmp (entry->d_name, "md-", sizeof ("md-") - 1) == 0)
              continue;
-           if (names_len >= names_max)
+           if (devs_len >= devs_max)
              {
-               names_max *= 2;
-               names = xrealloc (names, names_max * sizeof (*names));
+               devs_max *= 2;
+               devs = xrealloc (devs, devs_max * sizeof (*devs));
              }
-           names[names_len++] = xasprintf (entry->d_name);
+           devs[devs_len].stable =
+             xasprintf ("/dev/disk/by-id/%s", entry->d_name);
+           devs[devs_len].kernel =
+             canonicalize_file_name (devs[devs_len].stable);
+           devs_len++;
          }
 
-       /* /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);
+       qsort (devs, devs_len, sizeof (*devs), &compare_devices);
 
        closedir (dir);
 
        /* Now add all the devices in sorted order.  */
-       for (i = 0; i < names_len; ++i)
+       for (i = 0; i < devs_len; ++i)
          {
-           char *path = xasprintf ("/dev/disk/by-id/%s", names[i]);
-           if (check_device_readable_unique (path))
+           if (check_device_readable_unique (devs[i].stable))
              {
-               if (hook (path, 0))
+               if (hook (devs[i].stable, 0))
                  goto out;
              }
-           free (path);
-           free (names[i]);
+           free (devs[i].stable);
+           free (devs[i].kernel);
          }
-       free (names);
+       free (devs);
       }
   }
 

Thanks,

-- 
Colin Watson                                       address@hidden