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

[Vesa Jääskeläinen]

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.