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