On Wed, Jun 26, 2024 at 7:58 AM Daniel Kiper <dki...@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 <simonp...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
