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

[Laszlo Ersek]

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 at redhat.com>

Thanks!
Laszlo

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