Bug#554790: This breaks device.map on upgrade
Colin Watson
cjwatson at ubuntu.com
Mon Jul 12 10:38:16 UTC 2010
[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 <vadic052 at gmail.com>
2010-07-12 Colin Watson <cjwatson at 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.
=== 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 [cjwatson at ubuntu.com]
More information about the Pkg-grub-devel
mailing list