Re: [PATCH 2/2] lvm: Add support for cachevol and integrity lv

[Daniel Kiper]

On Sun, Jun 09, 2024 at 03:35:06PM -0400, 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
>
> Signed-off-by: Patrick Plenefisch <simonpatp@gmail.com>
> ---
>  grub-core/disk/diskfilter.c |  6 ++-
>  grub-core/disk/lvm.c        | 85 +++++++++++++++++++++++--------------
>  2 files changed, 57 insertions(+), 34 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)
> diff --git a/grub-core/disk/lvm.c b/grub-core/disk/lvm.c
> index 10bc965a4..17e225596 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;
>              }
>              }
> +          /* Cache and integrity LVs have extra parts that
> +           * we can ignore for our read-only access */

Please stick to the GRUB coding style [1]...

>            else if (grub_memcmp (p, "cache\"",
> -                   sizeof ("cache\"") - 1) == 0)
> +                   sizeof ("cache\"") - 1) == 0
> +                   || grub_memcmp (p, "cache+CACHE_USES_CACHEVOL\"",
> +                   sizeof ("cache+CACHE_USES_CACHEVOL\"") - 1) == 0
> +                   || grub_memcmp (p, "integrity\"",
> +                   sizeof ("integrity\"") - 1) == 0)

I think grub_strncmp() instead of grub_memcmp() would be more natural here.

>              {
>                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
>
>                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';

OK but this change should be mentioned in the commit message, i.e. you
do this on the occasion.

>                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,46 @@ grub_lvm_detect (grub_disk_t disk,
>              ignored_feature->lv = NULL;
>            }
>            }
> +          else
> +          {
> +

Please drop this empty line.

May I ask you to send patches as a reply to a cover letter?
If you use "git send-email" it will do it for you.

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments