Re: [PATCH] simplify grub_raid_array

[Jeroen Dekkers <jeroen@vrijschrift.org>]

At Wed, 6 Feb 2008 23:59:54 +0100,
Robert Millan wrote:
> 
> On Wed, Feb 06, 2008 at 11:43:39PM +0100, Jeroen Dekkers wrote:
> > At Wed, 6 Feb 2008 17:45:07 +0100,
> > Robert Millan wrote:
> > > Unless I missed something, it seems that grub_raid_array contains redundant
> > > information (`name' is already present via `disk->name').  I propose to
> > > simplify it this way.
> > 
> > No idea why, I don't have the time to look at the actual code, but 
> >  
> > > @@ -410,7 +410,7 @@ grub_raid_scan_device (const char *name)
> > >  	  return 0;
> > >  	}
> > >    
> > > -      if (array->device[sb.this_disk.number].name != 0)
> > > +      if (array->device[sb.this_disk.number]->name != 0)
> > >  	{
> > >  	  /* We found multiple devices with the same number. Again,
> > >  	     this shouldn't happen.*/
> > 
> > looks suspicious to me. Is that really doing what it is meant to do?
> 
> Yes.  

No, it doesn't. The meaning of the check .name != 0 is whether a
device with that number already exists in the array. If .name is 0, it
doesn't exist yet, because else it would've been assigned
something. What you're now doing is wrong because if there isn't a
device in the array then array->device[sb.this_disk.number] is 0. I
guess the only reason this actually works is because the first page of
memory (the one at address 0) happens to be zero by luck.


Jeroen Dekkers