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

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 07/12/2010 12:38 PM, Colin Watson wrote:
> [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.
>>     
>   
No need of copyright assignment for this patch.
> 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.
>
>   
>
> 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.
>
>   
Go ahead.
> === 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,
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature