Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

["Vesa Jääskeläinen" <chaac@nic.fi>]

Felix Zielcke wrote:
> Am Dienstag, den 02.09.2008, 00:46 +0200 schrieb Felix Zielcke:
> 
>> I'm too lazy now to make a new patch and go sleeping now.
>> [...]
>>
>> I hope that Marco could have a quick look over it especially the
>> changelog part :)
> 
> Final patch attached.
> In changelog I had again past and present mixed and I just use now the
> `normal' types instead of grub_uintN_t, seems like that's more used on
> util/
> So if there are no objections I'd like to commit this :)

> +	unsigned short i, j;
> +	const unsigned char k = sizeof ("/dev/mapper/") - 1;
> +	const unsigned short l = strlen (os_dev) - k + 1;

sizeof returns type of size_t so it would be good that char k uses that.
I am a bit surprised that this didn't generate compiler warning? And
there is no reason to define integers as constants unless you really
want to make sure they don't change :)

I assume we have grub_size_t or comparable there.

> +	const unsigned short l = strlen (os_dev) - k + 1;
>  
> -      break;
> +	grub_dev = xmalloc (strlen (os_dev) - k + 1);

if you already calculate something to variable it would be nice to
re-use that calculation again.

> +	for (i = 0, j = 0; i < l; i++, j++)
> +	  {
> +	    grub_dev[j] = os_dev[k + i];
> +	    if (os_dev[k + i] == '-' && os_dev[k + i + 1] == '-')
> +	      i++;
> +	  }

Can't you index i: k <= i < strlen(os_dev)? Would make this a bit more
readable. Or as you could just increment k in every loop and use that to
index. Your 0 <= j < l should limit char array.

You could use a bit more descriptive names like l->len, k->start/offset.
For indexes we quite often use i,j,k.

> +	  p = strchr (os_dev, 'p');
> +	  if (p)
> +	    *p = ',';

It is usually a bad idea to modify source string.