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

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                                       [cjwatson@debian.org]

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

[Colin Watson]

[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@gmail.com>
2010-07-12  Colin Watson  <cjwatson@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@ubuntu.com]

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 6349 bytes --]

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  <vadic052@gmail.com>
> 2010-07-12  Colin Watson  <cjwatson@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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Colin Watson]

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  <vadic052@gmail.com>
> > 2010-07-12  Colin Watson  <cjwatson@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                                       [cjwatson@ubuntu.com]