Re: [PATCH 3/4 v11] lvm: Add support for integrity lv

[Daniel Kiper]

On Mon, Nov 11, 2024 at 01:32:59PM -0500, Patrick Plenefisch wrote:
> lv matching must be done after processing the ignored feature
> indirections, as integrity volumes & caches may have several levels
> of indirection that the segments must be shifted through.
>
> pv matching must be completely finished before validating a
> volume, otherwise referenced raid stripes may not have pv
> data applied yet
>
> This patch contains a change requested by Daniel Kiper to use
> a null character instead of an integer zero to terminate strings
>
> Signed-off-by: Patrick Plenefisch <simonpatp@gmail.com>
> ---
>  grub-core/disk/diskfilter.c |  6 ++-
>  grub-core/disk/lvm.c        | 86 +++++++++++++++++++++++--------------
>  2 files changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
> index 21e239511..dc3bd943b 100644
> --- a/grub-core/disk/diskfilter.c
> +++ b/grub-core/disk/diskfilter.c
> @@ -966,8 +966,6 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg 
> *vg)
>
>    for (lv = vg->lvs; lv; lv = lv->next)
>      {
> -      grub_err_t err;
> -
>        /* RAID 1 and single-disk RAID 0 don't use a chunksize but code
>           assumes one so set one. */
>        for (i = 0; i < lv->segment_count; i++)
> @@ -979,6 +977,10 @@ grub_diskfilter_vg_register (struct grub_diskfilter_vg 
> *vg)
>            && lv->segments[i].stripe_size == 0)
>          lv->segments[i].stripe_size = 64;
>      }
> +    }
> +  for (lv = vg->lvs; lv; lv = lv->next)
> +    {
> +      grub_err_t err;
>
>        err = validate_lv(lv);
>        if (err)

The grub-core/disk/diskfilter.c change begs for separate patch...

> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 286132d74..abb5b12ae 100644
> --- a/grub-core/disk/lvm.c
> +++ b/grub-core/disk/lvm.c
> @@ -805,13 +805,27 @@ grub_lvm_detect (grub_disk_t disk,
>                seg->nodes[seg->node_count - 1].name = tmp;
>              }
>              }
> -          else if (grub_memcmp (p, "cache\"",
> -                   sizeof ("cache\"") - 1) == 0)
> +          /*
> +           * Cache and integrity LVs have extra parts that
> +           * we can ignore for our read-only access
> +           */
> +          else if (grub_strncmp (p, "cache\"",
> +                   sizeof ("cache\"") - 1) == 0
> +                   || grub_strncmp (p, "integrity\"",
> +                   sizeof ("integrity\"") - 1) == 0)
>              {
>                struct ignored_feature_lv *ignored_feature = NULL;
>
>                char *p2, *p3;
>                grub_size_t sz;
> +#ifdef GRUB_UTIL
> +              p2 = grub_strchr (p, '"');
> +              if (p2)
> +            *p2 = '\0';
> +              grub_util_info ("Ignoring extra metadata type '%s' for
> %s", p, lv->name);
> +              if (p2)
> +            *p2 ='"';
> +#endif

This change does not seem to belong to this patch...

>                ignored_feature = grub_zalloc (sizeof (*ignored_feature));
>                if (!ignored_feature)
> @@ -892,7 +906,7 @@ grub_lvm_detect (grub_disk_t disk,
>                char *p2;
>                p2 = grub_strchr (p, '"');
>                if (p2)
> -            *p2 = 0;
> +            *p2 = '\0';
>                grub_util_info ("unknown LVM type %s", p);
>                if (p2)
>              *p2 ='"';
> @@ -936,36 +950,6 @@ grub_lvm_detect (grub_disk_t disk,
>          }
>      }
>
> -      /* Match lvs.  */
> -      {
> -    struct grub_diskfilter_lv *lv1;
> -    struct grub_diskfilter_lv *lv2;
> -    for (lv1 = vg->lvs; lv1; lv1 = lv1->next)
> -      for (i = 0; i < lv1->segment_count; i++)
> -        for (j = 0; j < lv1->segments[i].node_count; j++)
> -          {
> -        if (vg->pvs)
> -          for (pv = vg->pvs; pv; pv = pv->next)
> -            {
> -              if (! grub_strcmp (pv->name,
> -                     lv1->segments[i].nodes[j].name))
> -            {
> -              lv1->segments[i].nodes[j].pv = pv;
> -              break;
> -            }
> -            }
> -        if (lv1->segments[i].nodes[j].pv == NULL)
> -          for (lv2 = vg->lvs; lv2; lv2 = lv2->next)
> -            {
> -              if (lv1 == lv2)
> -                continue;
> -              if (grub_strcmp (lv2->name,
> -                       lv1->segments[i].nodes[j].name) == 0)
> -            lv1->segments[i].nodes[j].lv = lv2;
> -            }
> -          }
> -
> -      }
>
>        {
>      struct ignored_feature_lv *ignored_feature;
> @@ -1014,9 +998,45 @@ grub_lvm_detect (grub_disk_t disk,
>              ignored_feature->lv = NULL;
>            }
>            }
> +          else
> +          {
> +#ifdef GRUB_UTIL
> +              grub_util_info ("Couldn't find LVM part of ignored
> feature on %s", ignored_feature->origin);
> +#endif
> +          }

Ditto...

Daniel