Re: [PATCH] add support for dmraid devices

[Felix Zielcke]

Am Mittwoch, den 17.06.2009, 18:44 -0400 schrieb Pavel Roskin: 
> On Wed, 2009-06-17 at 16:08 +0200, Felix Zielcke wrote:
> > add support for dmraid devices
> 
> That's good.  I have a system with a PDC RAID, and although I only have
> one drive connected, GRUB2 won't install on it.  I'm looking forward to
> testing your patch on that hardware.
> 
> Unfortunately, your patch doesn't compile as is.  That hanging "12" in
> grub_util_is_dmraid() is not needed.  Also please avoid adding trailing
> whitespace.  STGit warns about it.  "hostdisc" should be spelled
> "hostdisk".

Ah right sorry. Fixed.

> What is "cediideh"?  Actually, on the system I mentioned, root is
> mounted on /dev/mapper/pdc_dieaihahp2.  If "cediideh" is supposed to
> represent all weird names 8 characters long, let's use numbers
> "12345678".

cediideh is how my nvidia dmraid device is called.
I changed it now to abcdefgh.

> Even after fixing the compile error, I'm getting a warning:
> 
> util/hostdisk.c: In function 'grub_util_biosdisk_get_grub_dev':
> util/hostdisk.c:916: warning: 'disk' may be used uninitialized in this
> function
> util/hostdisk.c:916: note: 'disk' was declared here
> 
> Your patch removed the code where 'disk' is initialized:
> disk = grub_disk_open (name);
> 
> Yet grub_disk_close() is still there.  Removing it fixes the warning,
> but I'd like you to recheck the patch.  Apparently the 'disk' variable
> was used for different purposes throughout the function.  While at that,
> it would be great to avoid variable shadowing too and keep variables in
> the innermost possible scope.

I don't get any warning at all with the Debian gcc 4.3.3.
Anyway I fixed it now.

> Finally, the test results, apparently negative:
> 

Here's a new one which adds full support for pdc devices.
> 
> I think we need a list of possible dmraid names.  There we could go
> through the list in a loop.  That applies to grub_util_is_dmraid() and
> other places in the code.

I don't think it would be that useful, because hostdisk.c
grub_util_biosdisk_get_grub_dev and device_is_wholedisk just can't use
this list directly.

dmraid.patch.2
Description: Text document