Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 16/9/22 11:27, Markus Armbruster wrote:
>> Claudio Fontana <cfontana@suse.de> writes:
>> 
>>> improve error handling during module load, by changing:
>>>
>>> bool module_load_one(const char *prefix, const char *lib_name);
>>> void module_load_qom_one(const char *type);
>>>
>>> to:
>>>
>>> bool module_load_one(const char *prefix, const char *name, Error **errp);
>>> bool module_load_qom_one(const char *type, Error **errp);
>>>
>>> module_load_qom_one has been introduced in:
>>>
>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>> module_load_one, but discarded the bool return value. Restore it.
>>>
>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>> as appropriate in each context.
>> 
>> How exactly does behavior change?  The commit message is mum on the
>> behavior before the patch, and vague on the behavior afterwards.
>> 
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>   audio/audio.c         |   9 ++-
>>>   block.c               |  15 ++++-
>>>   block/dmg.c           |  18 +++++-
>>>   hw/core/qdev.c        |  10 ++-
>>>   include/qemu/module.h |  38 ++++++++++--
>>>   qom/object.c          |  18 +++++-
>>>   softmmu/qtest.c       |   6 +-
>>>   ui/console.c          |  18 +++++-
>>>   util/module.c         | 140 ++++++++++++++++++++++++------------------
>>>   9 files changed, 194 insertions(+), 78 deletions(-)
>
>>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>>> index 8c012bbe03..78d4c4de96 100644
>>> --- a/include/qemu/module.h
>>> +++ b/include/qemu/module.h
>>> @@ -61,16 +61,44 @@ typedef enum {
>
>>>   
>>>   void module_call_init(module_init_type type);
>>> -bool module_load_one(const char *prefix, const char *lib_name);
>>> -void module_load_qom_one(const char *type);
>>> +
>>> +/*
>>> + * module_load_one: attempt to load a module from a set of directories
>>> + *
>>> + * directories searched are:
>>> + * - getenv("QEMU_MODULE_DIR")
>>> + * - get_relocated_path(CONFIG_QEMU_MODDIR);
>>> + * - /var/run/qemu/${version_dir}
>>> + *
>>> + * prefix:         a subsystem prefix, or the empty string ("audio-", ..., "")
>>> + * name:           name of the module
>>> + * errp:           error to set in case the module is found, but load failed.
>>> + *
>>> + * Return value:   true on success (found and loaded);
>>> + *                 if module if found, but load failed, errp will be set.
>>> + *                 if module is not found, errp will not be set.
>> 
>> I understand you need to distingush two failure modes "found, but load
>> failed" and "not found".
>> 
>> Functions that set an error on some failures only tend to be awkward: in
>> addition to checking the return value for failure, you have to check
>> @errp for special failures.  This is particularly cumbersome when it
>> requires a @local_err and an error_propagate() just for that.  I
>> generally prefer to return an error code and always set an error.
>
> I notice the same issue, therefore would suggest this alternative
> prototype:
>
>    bool module_load_one(const char *prefix, const char *name, 
>              bool ignore_if_missing, Error **errp);
> which always sets *errp when returning false:
>
>   * Return value:   if ignore_if_missing is true:
>   *                   true on success (found or missing), false on
>   *                   load failure.
>   *                 if ignore_if_missing is false:
>   *                   true on success (found and loaded); false if
>   *                   not found or load failed.
>   *                 errp will be set if the returned value is false.
>   */

I think this interface is less surprising.

If having to pass a flag turns out to to be a legibility issue, we can
have wrapper functions.