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

[Colin Watson]

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.

> --- 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);
>        }
>    }
>  

Thanks,

-- 
Colin Watson                                       address@hidden