Re: [PATCH] simplify grub_raid_array

[Robert Millan]

On Fri, Feb 08, 2008 at 11:56:07PM +0100, Jeroen Dekkers wrote:
> 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.

Yep.  I just fixed this a while ago (Sam caught that in a segfault).

Does it look good now?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)