Philippe Mathieu-Daudé <f4...@amsat.org> writes:
> On 16/9/22 11:27, Markus Armbruster wrote:
>> Claudio Fontana <cfont...@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 <cfont...@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.
