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

[Daniel Kiper]

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.

> 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/,/./

> in addition typing passphase for every system startup can be a big

s/in addition/Additionally,/

> 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/

> +      if ((pv = seg->nodes[j].pv))

if (seg->nodes[j].pv != NULL)

...and then later...

  pv = seg->nodes[j].pv

>       {
> -       /* 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.

> +           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...

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

!grub_strcmp() instead of grub_strcmp() == 0

> +           continue;

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

> +
> +       tmp = grub_malloc (sizeof (*tmp));

Please do not ignore grub_malloc() failures.

Daniel