Re: [PATCH] diskfilter: use nodes in logical volume's segment as member device

[Michael Chang <mchang@suse.com>]

On Mon, Aug 09, 2021 at 05:34:41PM +0200, Daniel Kiper wrote:
> On Mon, Aug 02, 2021 at 05:40:20PM +0800, Michael Chang via Grub-devel wrote:
> > Currently the grub_diskfilter_memberlist function returns all physical
> > volumes added to a volume group to which a logical volume (LV) belongs.
> > However this is suboptimal as it doesn't fit the intended behavior of
> > returning underlying devices that make up the LV. To give a clear
> > picture, the result should be identical to running commands below to
> > display the logical volumes with underlying physical volumes in use.
> >
> >   lvs -o +devices /dev/system/root
> >   lvdisplay --maps /dev/system/root
> 
> May I ask you to provide output from these two commands too? Additionally,
> I think it would be useful to have output from "pvs" and "lvs" commands
> in the commit message. I know "lsblk" shows you most information but
> I think both "pvs" and "lvs" are giving more readable output.

It sounds good as that helps better understanding. I'll add these
information to next version.

> 
> > This change is required if any part of the PV not used by root LV is
> > encrypted. Using this lvm setup as an example:
> >
> >   localhost:~ # lsblk
> >   NAME            MAJ:MIN RM  SIZE RO TYPE  MOUNTPOINT
> >   vda             253:0    0   20G  0 disk
> >   ├─vda1          253:1    0    8M  0 part
> >   └─vda2          253:2    0   20G  0 part
> >     ├─system-swap 254:0    0  1.4G  0 lvm   [SWAP]
> >     └─system-root 254:1    0 18.6G  0 lvm   /
> >   vdb             253:16   0   10M  0 disk
> >   └─data          254:2    0    8M  0 crypt
> >     └─system-data 254:3    0    4M  0 lvm   /data
> >
> > Running grub-install would end up with error because "system" VG
> > contains /dev/vdb which is encrypted.
> >
> >   error: attempt to install to encrypted disk without cryptodisk
> >   enabled. Set `GRUB_ENABLE_CRYPTODISK=y' in file `/etc/default/grub'.
> >
> > Certainly we can enable GRUB_ENABLE_CRYPTODISK=y and move on, but that
> > is not always acceptable since the server may need to boot unattended,
> 
> s/,/./

OK.

> 
> > in addition typing passphase for every system startup can be a big
> 
> s/in addition/Additionally,/

OK.

> 
> > hassle of which most users would like to avoid.
> >
> > This patch solves the problem by returning physical volumes, /dev/vda2,
> > rightly used by system-root in the example above, thus changing how
> > grub-install perceives the underlying block device to boot and avoids
> > the error from happening.
> >
> > Signed-off-by: Michael Chang <mchang@suse.de>
> > Tested-by: Olav Reinert <seroton10@gmail.com>
> > ---
> >  grub-core/disk/diskfilter.c | 44 +++++++++++++++++++++++--------------
> >  1 file changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> > index 6eb2349a6..595f5f70f 100644
> > --- a/grub-core/disk/diskfilter.c
> > +++ b/grub-core/disk/diskfilter.c
> > @@ -300,6 +300,8 @@ grub_diskfilter_memberlist (grub_disk_t disk)
> >    grub_disk_dev_t p;
> >    struct grub_diskfilter_vg *vg;
> >    struct grub_diskfilter_lv *lv2 = NULL;
> > +  struct grub_diskfilter_segment *seg;
> > +  unsigned int i, j;
> >
> >    if (!lv->vg->pvs)
> >      return NULL;
> > @@ -331,25 +333,33 @@ grub_diskfilter_memberlist (grub_disk_t disk)
> >  	    }
> >      }
> >
> > -  for (pv = lv->vg->pvs; pv; pv = pv->next)
> > -    {
> > -      if (!pv->disk)
> > +  for (i = 0, seg = lv->segments; i < lv->segment_count; i++, seg++)
> > +    for (j =0; j < seg->node_count; ++j)
> 
> s/=0/= 0/

OK.

> 
> > +      if ((pv = seg->nodes[j].pv))
> 
> if (seg->nodes[j].pv != NULL)
> 
> ...and then later...
> 
>   pv = seg->nodes[j].pv

OK.

> 
> >  	{
> > -	  /* TRANSLATORS: This message kicks in during the detection of
> > -	     which modules needs to be included in core image. This happens
> > -	     in the case of degraded RAID and means that autodetection may
> > -	     fail to include some of modules. It's an installation time
> > -	     message, not runtime message.  */
> > -	  grub_util_warn (_("Couldn't find physical volume `%s'."
> > -			    " Some modules may be missing from core image."),
> > -			  pv->name);
> > -	  continue;
> > +
> > +	  if (!pv->disk)
> > +	    {
> > +	      /* TRANSLATORS: This message kicks in during the detection of
> > +		 which modules needs to be included in core image. This happens
> > +		 in the case of degraded RAID and means that autodetection may
> > +		 fail to include some of modules. It's an installation time
> > +		 message, not runtime message.  */
> 
> Please fix this comment formatting if you move it.

May I ask what is wrong as it looks good to the indention that has been
adjusted to the new place ?

> 
> > +	      grub_util_warn (_("Couldn't find physical volume `%s'."
> > +				" Some modules may be missing from core image."),
> > +			      pv->name);
> > +	      continue;
> > +	    }
> > +
> > +	  for (tmp = list; tmp; tmp = tmp->next)
> 
> tmp != NULL please...

OK.

> 
> > +	    if (grub_strcmp (tmp->disk->name, pv->disk->name) == 0)
> 
> !grub_strcmp() instead of grub_strcmp() == 0

OK.

Well. It appears "grub_strcmp() == 0" more popular as it has more
grep hits . But I am fine in doing either way. :)

 grep -R '!grub_strcmp' | wc -l
 26
 grep -R 'grub_strcmp.*== 0' | wc -l
 385

> 
> > +	      continue;
> 
> I know what you mean but this "continue" does not make much sense here...

Indeed this looks bogus. I will fix this in next version.

> 
> > +
> > +	  tmp = grub_malloc (sizeof (*tmp));
> 
> Please do not ignore grub_malloc() failures.

OK.

Thanks a lot for review.

Michael

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel