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

[Claudio Fontana]

On 9/21/22 14:16, Markus Armbruster wrote:
> 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.
> 

To me it seems even more confusing tbh. Do we have more ideas? Richard?

bool module_load_one(const char *prefix, const char *name, Error **errp);

In my mind we should really say,

RETURN VALUE: a bool with the meaning: "was a module loaded? true/false"

ERROR: The Error parameter tells us: "was there an error loading the module?"

Makes sense to me, but maybe someone has a better one.

Btw, as an aside, for the general Error API pattern, if the bool return value 
and Error != NULL should be always related 1:1,
It would have been better to design it with only one of those informing about 
the error (Error, as it carries the additional Error information, besides the 
information that Error != NULL),
and remove the extra degree of freedom for a return value that at this point 
(in the current code) carries ZERO additional useful information.

We could have designed the API to use the return value as... an actual return 
value for solving the domain problem at hand,
and use only the Error parameter for the error path.

Ie the API standard pattern could have been, instead of bool function(Error **),

return_value_type_t function_that_can_fail(function_arguments, ..., Error 
**errp);

and then leave both return_value_type_t and the function_arguments for the 
normal function operation.

rv = function_that_can_fail(errp);
if (errp != NULL) {
    /* handle the error */
}
if (rv > 7) {
    /* handle the return value */
}

Would it not be better to handle the Error path and the normal return value 
separately?

With this pattern in mind, this specific case would then not be surprising to 
anyone,
and we would not have to start cooking up booleans to start passing to each 
function to say how errors should be treated,
as nobody would expect the bool returned to mean anything related with the 
Error path.

But this again would be rethinking the whole Error API.

We should in any case do the right thing in this specific case, and this 
specific case (handling module load errors vs modules not installed),
is not served well by the current code, whether it went through this attentive 
abstract principles scrutiny or not (seems not to me).

Thanks,

Claudio