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 <rjo...@redhat.com> Thanks! Laszlo > >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2168506 >> Signed-off-by: Laszlo Ersek <ler...@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. > _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
