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

[Patrick Plenefisch]

On Wed, Jun 26, 2024 at 7:58 AM Daniel Kiper <dkiper@net-space.pl> wrote:
>
> 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]...

I'm going to need more detail. I changed it to follow the style with
this patch, and I'm not sure what is wrong with it.

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