grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] osdep/linux: Fix md array device enumeration


From: Petr Vorel
Subject: Re: [PATCH] osdep/linux: Fix md array device enumeration
Date: Fri, 8 Oct 2021 10:25:37 +0200

Hi Kees,

> Hi Petr,

> On Wed, Oct 06, 2021 at 09:28:32AM +0200, Petr Vorel wrote:
> > Hi Kees, Daniel,

> > > On Sat, Sep 25, 2021 at 07:03:35PM -0700, kees@ubuntu.com wrote:
> > > > From: Kees Cook <kees@ubuntu.com>

> > > > GET_ARRAY_INFO's info.nr_disks does not map to GET_DISK_INFO's
> > > > disk.number, which is an internal kernel index. If an array has had 
> > > > drives
> > > > added, removed, etc, there may be gaps in GET_DISK_INFO's results. But
> > > > since the consumer of devicelist cannot tolerate gaps (it expects to 
> > > > walk
> > > > a NULL-terminated list of device name strings), the devicelist index (j)
> > > > must be tracked separately from the disk.number index (i). But grub
> > > > wants to only examine active (i.e. present and non-failed) disks, so how
> > > > many disks are left to be queried must be also separately tracked
> > > > (remaining).

> > Kees, thanks a lot for taking care for this.

> You bet! I seem to find some amazing corner cases. ;)
Yep, but both of your bug reports are really worth of fixing.


> > <snip>
> > > > +      /* Skip empty slot: MD_DISK_REMOVED slots don't count toward 
> > > > nr_disks. */
> > > >        if (disk.state & (1 << MD_DISK_REMOVED))
> > > >         continue;
> > > > +      remaining--;

> > > > -      if (disk.state & (1 << MD_DISK_ACTIVE))
> > > > -       devicelist[j] = grub_find_device (NULL,
> > > > -                                         makedev (disk.major, 
> > > > disk.minor));
> > > > -      else
> > > > -       devicelist[j] = NULL;
> > > > -      j++;
> > > > +      /* Only examine disks that are actively participating in the 
> > > > array. */
> > > > +      if (!(disk.state & (1 << MD_DISK_ACTIVE)))
> > > > +       continue;
> > > > +
> > > > +      devicelist[j++] = grub_find_device (NULL, makedev (disk.major,
> > > > +                                                        disk.minor));

> > > I would prefer if you leave original if statement as is and update
> > > grub_find_device() call accordingly... And of course drop else as
> > > needed... :-)
> > I suppose you'll need to send v2 due this, but for now:

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> Thanks! I've included your reviewed-by in the v2; hopefully that's okay
> as the changes were just stylistic.
Sure, changes in v2 are really minor + already asked to do by Daniel.
Thanks!

Kind regards,
Petr

> -Kees



reply via email to

[Prev in Thread] Current Thread [Next in Thread]