Re: [Libguestfs] [libguestfs-common PATCH 1/2] options/keys: key_store_import_key(): un-constify "key" parameter
On 5/16/23 14:14, Richard W.M. Jones wrote:
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?
Indeed you can have a const struct containing non-const strings, and the
next patch would still work fine if we dropped this patch.
The problem is with the "messaging" to the programmer. If a structure is
taken (via a pointer-to-const) by a function, that's effectively a
promise to the caller that neither the structure, nor anything reachable
from it, is going to be modified by the function. C doesn't have a way
to express the "anything reachable from it" part. "const" is
technically
not as effective as it should arguably be, but it carries a message, and
that message is not factual in this particular case (even before
patch#2) -- hence patch#1.
Slightly related: <https://c-faq.com/ansi/constmismatch.html>;.
However I agree it looks
confusing so that's a decent reason to change it, so:
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Thanks!
Laszlo
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2168506
> Signed-off-by: Laszlo Ersek <lersek(a)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.