Libguestfs - Sep 2022 - can not get the virt-sparsify code in libguestfs ?

On 09/23/22 16:19, Richard W.M. Jones wrote:
> From 98f0b3565457c08d14e1f9ab2acecea003ebf6e1 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones at redhat.com>
> Date: Fri, 23 Sep 2022 15:18:43 +0100
> Subject: [PATCH] options: Don't attempt to scan or open LVs if
"lvm2" feature
>  is not available
> 
> Reported-by: Feng Li
> ---
>  options/decrypt.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/options/decrypt.c b/options/decrypt.c
> index 6fc7760e3..e9d7d99e0 100644
> --- a/options/decrypt.c
> +++ b/options/decrypt.c
> @@ -205,19 +205,22 @@ inspect_do_decrypt (guestfs_h *g, struct key_store
*ks)
>    CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions
(g);
>    CLEANUP_FREE_STRING_LIST char **lvs = NULL;
>    bool need_rescan;
> +  const char *lvm2_feature[] = { "lvm2", NULL };
>  
>    if (partitions == NULL)
>      exit (EXIT_FAILURE);
>  
>    need_rescan = decrypt_mountables (g, (const char * const *)partitions,
ks);
>  
> -  if (need_rescan) {
> -    if (guestfs_lvm_scan (g, 1) == -1)
> +  if (guestfs_feature_available (g, (char **) lvm2_feature) > 0) {
> +    if (need_rescan) {
> +      if (guestfs_lvm_scan (g, 1) == -1)
> +        exit (EXIT_FAILURE);
> +    }
> +
> +    lvs = guestfs_lvs (g);
> +    if (lvs == NULL)
>        exit (EXIT_FAILURE);
> +    decrypt_mountables (g, (const char * const *)lvs, ks);
>    }
> -
> -  lvs = guestfs_lvs (g);
> -  if (lvs == NULL)
> -    exit (EXIT_FAILURE);
> -  decrypt_mountables (g, (const char * const *)lvs, ks);
>  }
So this is interesting.

(1) If the problem is that guestfs_lvs() is called without checking for
the lvm2 feature group first, then the bug was introduced in
libguestfs-common commit 2d8c0f8d40b5 ("options: decrypt LUKS-on-LV
devices", 2022-02-28). The manual indeed states that guestfs_lvs()
depends on the lvm2 feature group.

(2) If, however, the problem is *also* that guestfs_lvm_scan() is called
without checking for said feature group, then the bug was introduced in
either

(2.1) super-antique libguestfs commit a232e62dcf50 ("fish: '-i'
option
automatically handles whole-disk encryption.", 2010-11-05), or

(2.2) less antique but still historical libguestfs commit 55dfcb2211a6
("New API: lvm_scan, deprecate vgscan (RHBZ#1602353).", 2018-07-26).

In either of those (2.*) cases, libguestfs-common commit 2d8c0f8d40b5
would only perpetuate the pre-existent bug. Put differently, the
guestfs_lvm_scan() call had had its use even before libguestfs-common
commit 2d8c0f8d40b5 (i.e., before LUKS-on-LV support), and it was
already afflicted by the bug.


Now, I'm not obsessing about this just so we can append a proper
"Fixes:" line to the commit message. Instead, I think the scope of the
fix (= the commit message & the code body of the patch both) depend on
the above determination.

The subject line and the body in this patch suggest that
guestfs_lvm_scan() should be avoided when "lvm2" is missing, but the
documentation does not imply that guestfs_lvm_scan() were invalid to
call in that case. And those feature group dependency statements in the
manual come from the generator.

Now, in "generator/actions_core.ml", a bunch of lvm2-related
operations
(such as "lvs" itself) specify

    optional = Some "lvm2";

but that is not the case with "lvm_scan".

If case (1) applies, then the current patch (code & subject line) are
too heavy-handed. That is, the guestfs_lvm_scan() call should not be
further restricted than it currently is (depending only on
"need_rescan") -- such a restriction would be inconsistent with the
fact
that the guestfs_lvm_scan() API does not actually depend on the "lvm2"
feature group.

If case (2) applies however, then, in addition to the current contents
of the patch, we should make the "lvm_scan" operation depend on the
lvm2
feature group, in the generator.

Laszlo

On Sun, Sep 25, 2022 at 08:21:19AM +0200, Laszlo Ersek
wrote:
> On 09/23/22 16:19, Richard W.M. Jones wrote:
> > From 98f0b3565457c08d14e1f9ab2acecea003ebf6e1 Mon Sep 17 00:00:00 2001
> > From: "Richard W.M. Jones" <rjones at redhat.com>
> > Date: Fri, 23 Sep 2022 15:18:43 +0100
> > Subject: [PATCH] options: Don't attempt to scan or open LVs if
"lvm2" feature
> >  is not available
> > 
> > Reported-by: Feng Li
> > ---
> >  options/decrypt.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/options/decrypt.c b/options/decrypt.c
> > index 6fc7760e3..e9d7d99e0 100644
> > --- a/options/decrypt.c
> > +++ b/options/decrypt.c
> > @@ -205,19 +205,22 @@ inspect_do_decrypt (guestfs_h *g, struct
key_store *ks)
> >    CLEANUP_FREE_STRING_LIST char **partitions =
guestfs_list_partitions (g);
> >    CLEANUP_FREE_STRING_LIST char **lvs = NULL;
> >    bool need_rescan;
> > +  const char *lvm2_feature[] = { "lvm2", NULL };
> >  
> >    if (partitions == NULL)
> >      exit (EXIT_FAILURE);
> >  
> >    need_rescan = decrypt_mountables (g, (const char * const
*)partitions, ks);
> >  
> > -  if (need_rescan) {
> > -    if (guestfs_lvm_scan (g, 1) == -1)
> > +  if (guestfs_feature_available (g, (char **) lvm2_feature) > 0) {
> > +    if (need_rescan) {
> > +      if (guestfs_lvm_scan (g, 1) == -1)
> > +        exit (EXIT_FAILURE);
> > +    }
> > +
> > +    lvs = guestfs_lvs (g);
> > +    if (lvs == NULL)
> >        exit (EXIT_FAILURE);
> > +    decrypt_mountables (g, (const char * const *)lvs, ks);
> >    }
> > -
> > -  lvs = guestfs_lvs (g);
> > -  if (lvs == NULL)
> > -    exit (EXIT_FAILURE);
> > -  decrypt_mountables (g, (const char * const *)lvs, ks);
> >  }
> 
> So this is interesting.
> 
> (1) If the problem is that guestfs_lvs() is called without checking for
> the lvm2 feature group first, then the bug was introduced in
> libguestfs-common commit 2d8c0f8d40b5 ("options: decrypt LUKS-on-LV
> devices", 2022-02-28). The manual indeed states that guestfs_lvs()
> depends on the lvm2 feature group.
> 
> (2) If, however, the problem is *also* that guestfs_lvm_scan() is called
> without checking for said feature group, then the bug was introduced in
> either
> 
> (2.1) super-antique libguestfs commit a232e62dcf50 ("fish:
'-i' option
> automatically handles whole-disk encryption.", 2010-11-05), or
> 
> (2.2) less antique but still historical libguestfs commit 55dfcb2211a6
> ("New API: lvm_scan, deprecate vgscan (RHBZ#1602353).",
2018-07-26).
It's quite possible the bug has existed for a long time.  Almost all
builds will have the lvm2 feature enabled (since it simply checks that
the "lvm" program exists and you really have to go out of your way to
uninstall that on most Linux distros).
> In either of those (2.*) cases, libguestfs-common commit 2d8c0f8d40b5
> would only perpetuate the pre-existent bug. Put differently, the
> guestfs_lvm_scan() call had had its use even before libguestfs-common
> commit 2d8c0f8d40b5 (i.e., before LUKS-on-LV support), and it was
> already afflicted by the bug.
> 
> 
> Now, I'm not obsessing about this just so we can append a proper
> "Fixes:" line to the commit message. Instead, I think the scope
of the
> fix (= the commit message & the code body of the patch both) depend on
> the above determination.
Yup, I need to improve the commit message.  I didn't even test the
patch beyond compiling it.  Hopefully the bug reporter will test it
soon.
> The subject line and the body in this patch suggest that
> guestfs_lvm_scan() should be avoided when "lvm2" is missing, but
the
> documentation does not imply that guestfs_lvm_scan() were invalid to
> call in that case. And those feature group dependency statements in the
> manual come from the generator.
> 
> Now, in "generator/actions_core.ml", a bunch of lvm2-related
operations
> (such as "lvs" itself) specify
> 
>     optional = Some "lvm2";
> 
> but that is not the case with "lvm_scan".
>
> If case (1) applies, then the current patch (code & subject line) are
> too heavy-handed. That is, the guestfs_lvm_scan() call should not be
> further restricted than it currently is (depending only on
> "need_rescan") -- such a restriction would be inconsistent with
the fact
> that the guestfs_lvm_scan() API does not actually depend on the
"lvm2"
> feature group.
> 
> If case (2) applies however, then, in addition to the current contents
> of the patch, we should make the "lvm_scan" operation depend on
the lvm2
> feature group, in the generator.
Looking briefly at do_lvm_scan in daemon/lvm.c, it seems as if it
ought to depend on the lvm2 feature.  Alternately (and less invasively
for callers) we might make it a no-op if the lvm2 feature is not
available, since scanning for LVM objects without LVM is a no-op.  But
yes it looks like this is another bug.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org