Bug#554790: This breaks device.map on upgrade
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 +
+++ util/deviceiter.c 2010-07-12 10:34:16 +
@@ -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 =
Bug#554790: This breaks device.map on upgrade
On Tue, Jul 20, 2010 at 03:02:47PM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote: On 07/12/2010 12:38 PM, Colin Watson wrote: 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. 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. OK, committed. Thanks. -- Colin Watson [cjwat...@ubuntu.com] -- To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#554790: This breaks device.map on upgrade
[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.0
+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 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.
Bug#554790: This breaks device.map on upgrade
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.0
+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 [cjwat...@debian.org]
--
To UNSUBSCRIBE, email to debian-bugs-rc-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#554790: This breaks device.map on upgrade
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.
--
Vadim Solomin
--- grub2-1.98+20100706/util/deviceiter.c 2010-07-06 20:57:30.0 +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);
}
}
