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

[Felix Zielcke]

Am Dienstag, den 02.09.2008, 00:14 +0200 schrieb Robert Millan:
> On Mon, Aug 18, 2008 at 05:05:26PM +0200, Felix Zielcke wrote:
> > +   unsigned char i, j, k, l;
> 
> I think using unsigned chars to store "integers" is counter-intuitive, and in
> some cases possibly dangerous (overflow).

I should have probable even used grub_uintN_t types.
16bit should be enough for them I think
255 chars in /dev/mapper/filename could be maybe a problem if people do
such weird things :)


> > +   grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
> > +
> > +   j = sizeof ("/dev/mapper/") -1;
> 
>                                      ^
> 
> Missing space here :-)

Yep and I even corrected this already in my last patch
http://lists.gnu.org/archive/html/grub-devel/2008-08/msg00523.htmlhttp://lists.gnu.org/archive/html/grub-devel/2008-08/msg00523.html

I'm now not that sure if that #define k would be okay, seems like Vesa
isn't liking macros.
But I could just use const for that too.

> > +   for (i = 0, k = 0; i < l; i++)
> > +     {
> > +       grub_dev[k] = os_dev[j + i];
> > +       k++;
> 
> i already counts from 0 and increments by-one.  Can it be used instead of k?

Oh I just noticed again that in my last patch it's now j
i is used as the source count and get's incremented twice for a dash so
one dash is skipped.
whereas k (new j) is used as the destination count so it's always only
incremented by one.

That's the whole problem, skip the next dash on a dash but don't skip a
single dash.
It's always
/dev/mapper/vg-lv
but if vg and lv part has a single dash then it's for example
v--g-l--v
for grub this has to be v-g-l-v

> The rest of the code I mostly don't understand well.  If you feel confident
> that it's right, I suggest you check it in unless someone else also wants to
> review it.
> 

Honestly I'm happy that the LVM part seems to work and didn't introduce
any problem.

I'm too lazy now to make a new patch and go sleeping now.
But the changes I do make tomorrow now on the last patch above is
making k a const instead of #define, i think it just looks better
and using grub_uint16_t for all 4 variables.

I hope that Marco could have a quick look over it especially the
changelog part :)

-- 
Felix Zielcke