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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
