On 12/7/23 16:08, Richard W.M. Jones wrote:

Use a named variable to make the boolean condition explicit. Pure refactoring, except applying De Morgan's law to avoid the confusing double negative condition. --- options/keys.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/options/keys.c b/options/keys.c index 52b2736..3a49273 100644 --- a/options/keys.c +++ b/options/keys.c @@ -20,6 +20,7 @@

#include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <unistd.h> #include <termios.h> #include <string.h> @@ -152,9 +153,12 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, if (ks) { for (i = 0; i < ks->nr_keys; ++i) { struct key_store_key *key = &ks->keys[i]; + bool key_id_matches_this_device;

- if (STRNEQ (key->id, device) && (!uuid || STRNEQ (key->id, uuid))) - continue; + key_id_matches_this_device = + STREQ (key->id, device) || + (uuid && STREQ (key->id, uuid)); + if (!key_id_matches_this_device) continue;

switch (key->type) { case key_string:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On 12/7/23 16:08, Richard W.M. Jones wrote:

We generally prefer to give specific parameters and avoid indirection. Therefore avoid defining "ID" and repeat the items instead.

Use B<> (bold) consistently for invariant parts of the option. --- options/key-option.pod | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/options/key-option.pod b/options/key-option.pod index 6bc04df..ee515fa 100644 --- a/options/key-option.pod +++ b/options/key-option.pod @@ -1,22 +1,30 @@ =item B<--key> SELECTOR

Specify a key for LUKS, to automatically open a LUKS device when using -the inspection. C<ID> can be either the libguestfs device name, or -the UUID of the LUKS device. +the inspection.

=over 4

-=item B<--key> C<ID>:key:KEY_STRING +=item B<--key> NAMEB<:key:>KEY_STRING + +=item B<--key> UUIDB<:key:>KEY_STRING + +C<NAME> is the libguestfs device name (eg. C</dev/sda1>). C<UUID> is +the device UUID.

Use the specified C<KEY_STRING> as passphrase.

-=item B<--key> C<ID>:file:FILENAME +=item B<--key> NAMEB<:file:>FILENAME + +=item B<--key> UUIDB<:file:>FILENAME

Read the passphrase from F<FILENAME>.

-=item B<--key> C<ID>:clevis +=item B<--key> NAMEB<:clevis>

-Attempt passphrase-less unlocking for C<ID> with Clevis, over the +=item B<--key> UUIDB<:clevis> + +Attempt passphrase-less unlocking for the device with Clevis, over the network. Please refer to L<guestfs(3)/ENCRYPTED DISKS> for more information on network-bound disk encryption (NBDE).

Should we use C<NAME> and C<UUID>? The removed part was C<ID>, not just ID. Reviewed-by: Laszlo Ersek <lersek@redhat.com>

On 12/7/23 16:08, Richard W.M. Jones wrote:

virt-v2v (and other virt tools using --key) forces you to supply keys on the command line per-device, eg. --key UUID:SELECTOR where UUID is the UUID of the encrypted device. In practice this requires users to pre-inspect guests to find out what device UUIDs they are using (or have some out-of-band method), match keys to UUIDs, and then pass the result to virt-v2v.

Obviously this is a pain in the neck for users, as predicted when the feature was initially added. Although it is more efficient for virt-v2v -- because attempting to decrypt a device with an invalid key is slow -- there is a significant number of VMs (probably the vast majority) that only have a single encrypted device, so this isn't an issue in most cases.

With a large number of devices and a matching number of "all" keys, the quadratic slowdown could become serious (on average N/2 keys tried for each of N devices). But, that depends on the VM to convert, and how the user chooses to pass in the keys. Not a counter-argument, just something to be aware of, perhaps.

This change allows --key all:SELECTOR to be used, to try the key

"--key all:SELECTOR" is not precise, considering the documentation; the option format is "--key SELECTOR", so "all:" is a part of (i.e., the prefix of) SELECTOR. Instead, This change enables "all:*" selectors to be used with with --key could be more precise. Or, well, not. :) Either way, this only concerns the commit message, not the code or the docs (and the series is upstream anyway). The patch looks good to me: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo

against any encrypted device encountered. --- options/key-option.pod | 9 ++++++++- options/keys.c | 1 + options/options.h | 2 ++ 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/options/key-option.pod b/options/key-option.pod index ee515fa..1470d86 100644 --- a/options/key-option.pod +++ b/options/key-option.pod @@ -9,8 +9,11 @@ the inspection.

=item B<--key> UUIDB<:key:>KEY_STRING

+=item B<--key> B<all:key:>KEY_STRING + C<NAME> is the libguestfs device name (eg. C</dev/sda1>). C<UUID> is -the device UUID. +the device UUID. C<all> means try the key against any encrypted +device.

Use the specified C<KEY_STRING> as passphrase.

@@ -18,12 +21,16 @@ Use the specified C<KEY_STRING> as passphrase.

=item B<--key> UUIDB<:file:>FILENAME

+=item B<--key> B<all:file:>FILENAME + Read the passphrase from F<FILENAME>.

=item B<--key> NAMEB<:clevis>

=item B<--key> UUIDB<:clevis>

+=item B<--key> B<all:clevis> + Attempt passphrase-less unlocking for the device with Clevis, over the network. Please refer to L<guestfs(3)/ENCRYPTED DISKS> for more information on network-bound disk encryption (NBDE). diff --git a/options/keys.c b/options/keys.c index 3a49273..87acba5 100644 --- a/options/keys.c +++ b/options/keys.c @@ -156,6 +156,7 @@ get_keys (struct key_store *ks, const char *device, const char *uuid, bool key_id_matches_this_device;

key_id_matches_this_device = + STREQ (key->id, "all") || /* special string "all" matches any device */ STREQ (key->id, device) || (uuid && STREQ (key->id, uuid)); if (!key_id_matches_this_device) continue; diff --git a/options/options.h b/options/options.h index 94e8b9e..dcb15c2 100644 --- a/options/options.h +++ b/options/options.h @@ -109,6 +109,8 @@ struct key_store_key { * device name, or the UUID. * * There may be multiple matching devices in the list. + * + * This may be the special string "all" which matches any device. */ char *id;