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" <rjo...@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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
