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

Thanks Richard and Ersek: We checked the LVM2 lib in already installed in system before we compile the libguestfs 1.48.4, but in virt-sparsify log , it still report "error: feature 'lvm2' is not available in thisbuild of libguestfs." We used the ./test-lvm-filtering.sh to test , and it said the LVM2 is not available in the build of libguestfs as well. root@weibupos:/home/intel/libguestfs-1.48.4/tests/lvm# ./test-lvm-filtering.sh *stdin*:9: libguestfs: error: pvcreate: feature 'lvm2' is not available in this build of libguestfs. Read 'AVAILABILITY' in the guestfs(3) man page for how to check for the availability of features. We used below steps to build the source: Refer the https://libguestfs.org/guestfs-building.1.html Download the 1.48.4 from the https://libguestfs.org/ and install the dependence . autoreconf -i ./configure CFLAGS=-fPIC make Any steps to get detail trace log to see why the LVM2 is not available in the libguestfs? -----Original Message----- From: Richard W.M. Jones <rjones@redhat.com> Sent: 2022年9月26日 15:43 To: Laszlo Ersek <lersek@redhat.com> Cc: Li, Feng F <feng.f.li@intel.com>; libguestfs@redhat.com Subject: Re: can not get the virt-sparsify code in libguestfs ? 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@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