Libguestfs - May 2023 - [libguestfs-common PATCH 1/2] options/keys: key_store_import_key(): un-constify "key" parameter

The key_store_import_key() function is called from both C-language
utilities -- via key_store_add_from_selector() -- and OCaml-language ones
-- via guestfs_int_mllib_inspect_decrypt().  We currently declare the
function's second parameter as

  const struct key_store_key *key

That is however a superficial, if not outright false, promise: in
key_store_import_key(), we take ownership of all three string fields
within "key", as evidenced by free_key_store(), where we free() all
three
strings. With the same effort, we might as well modify the contents of
those strings at once. Drop "const". (None of the callers care, but
let's
be honest.)

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2168506
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 options/options.h | 3 ++-
 options/keys.c    | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/options/options.h b/options/options.h
index 94573ee063bb..94e8b9eef43e 100644
--- a/options/options.h
+++ b/options/options.h
@@ -169,7 +169,8 @@ extern struct matching_key *get_keys (struct key_store *ks,
const char *device,
                                       const char *uuid, size_t *nr_matches);
 extern void free_keys (struct matching_key *keys, size_t nr_matches);
 extern struct key_store *key_store_add_from_selector (struct key_store *ks,
const char *selector);
-extern struct key_store *key_store_import_key (struct key_store *ks, const
struct key_store_key *key);
+extern struct key_store *key_store_import_key (struct key_store *ks,
+                                               struct key_store_key *key);
 extern bool key_store_requires_network (const struct key_store *ks);
 extern void free_key_store (struct key_store *ks);
 
diff --git a/options/keys.c b/options/keys.c
index 48f1bc7c7c47..bc7459749276 100644
--- a/options/keys.c
+++ b/options/keys.c
@@ -261,7 +261,7 @@ key_store_add_from_selector (struct key_store *ks, const
char *selector)
 }
 
 struct key_store *
-key_store_import_key (struct key_store *ks, const struct key_store_key *key)
+key_store_import_key (struct key_store *ks, struct key_store_key *key)
 {
   struct key_store_key *new_keys;

On Mon, May 15, 2023 at 07:49:23PM +0200, Laszlo Ersek
wrote:
> The key_store_import_key() function is called from both C-language
> utilities -- via key_store_add_from_selector() -- and OCaml-language ones
> -- via guestfs_int_mllib_inspect_decrypt().  We currently declare the
> function's second parameter as
> 
>   const struct key_store_key *key
> 
> That is however a superficial, if not outright false, promise: in
> key_store_import_key(), we take ownership of all three string fields
> within "key", as evidenced by free_key_store(), where we free()
all three
> strings. With the same effort, we might as well modify the contents of
> those strings at once. Drop "const". (None of the callers care,
but let's
> be honest.)
I'm not completely certain what the rules are here.  Can't you have a
const struct containing non-const strings?  However I agree it looks
confusing so that's a decent reason to change it, so:

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2168506
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>  options/options.h | 3 ++-
>  options/keys.c    | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/options/options.h b/options/options.h
> index 94573ee063bb..94e8b9eef43e 100644
> --- a/options/options.h
> +++ b/options/options.h
> @@ -169,7 +169,8 @@ extern struct matching_key *get_keys (struct key_store
*ks, const char *device,
>                                        const char *uuid, size_t
*nr_matches);
>  extern void free_keys (struct matching_key *keys, size_t nr_matches);
>  extern struct key_store *key_store_add_from_selector (struct key_store
*ks, const char *selector);
> -extern struct key_store *key_store_import_key (struct key_store *ks, const
struct key_store_key *key);
> +extern struct key_store *key_store_import_key (struct key_store *ks,
> +                                               struct key_store_key *key);
>  extern bool key_store_requires_network (const struct key_store *ks);
>  extern void free_key_store (struct key_store *ks);
>  
> diff --git a/options/keys.c b/options/keys.c
> index 48f1bc7c7c47..bc7459749276 100644
> --- a/options/keys.c
> +++ b/options/keys.c
> @@ -261,7 +261,7 @@ key_store_add_from_selector (struct key_store *ks,
const char *selector)
>  }
>  
>  struct key_store *
> -key_store_import_key (struct key_store *ks, const struct key_store_key
*key)
> +key_store_import_key (struct key_store *ks, struct key_store_key *key)
>  {
>    struct key_store_key *new_keys;
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v