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

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- 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 --]