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 <vadic...@gmail.com>
> 2010-07-12 Colin Watson <cjwat...@ubuntu.com>
>
> 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
