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

[Markus Armbruster]

Claudio Fontana <cfontana@suse.de> writes:

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

We did in the beginning, although only tacitly.  A deviation from
Error's inspiration GError (we rolled our own because we didn't use GLib
back then).

This deviation turned out to make Error cumbersome to use.  Which then
bred inconsistent usage, which led to confusion about how it should be
used, which complicated review, which finally made me bite the bullet
and correct the mistake:

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

This part of error.h's big comment goes back to

commit e3fe3988d7851cac30abffae06d2f555ff7bee62
Author: Markus Armbruster <armbru@redhat.com>
Date:   Tue Jul 7 18:05:31 2020 +0200

    error: Document Error API usage rules
    
    This merely codifies existing practice, with one exception: the rule
    advising against returning void, where existing practice is mixed.
    
    When the Error API was created, we adopted the (unwritten) rule to
    return void when the function returns no useful value on success,
    unlike GError, which recommends to return true on success and false on
    error then.
    
    When a function returns a distinct error value, say false, a checked
    call that passes the error up looks like
    
        if (!frobnicate(..., errp)) {
            handle the error...
        }
    
    When it returns void, we need
    
        Error *err = NULL;
    
        frobnicate(..., &err);
        if (err) {
            handle the error...
            error_propagate(errp, err);
        }
    
    Not only is this more verbose, it also creates an Error object even
    when @errp is null, &error_abort or &error_fatal.

Note: same issue when it returns something without a recognizable error
value.
    
    People got tired of the additional boilerplate, and started to ignore
    the unwritten rule.  The result is confusion among developers about
    the preferred usage.
    
    Make the rule advising against returning void official by putting it
    in writing.  This will hopefully reduce confusion.
    
    Update the examples accordingly.
    
    The remainder of this series will update a substantial amount of code
    to honor the rule.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Reviewed-by: Greg Kurz <groug@kaod.org>
    Message-Id: <20200707160613.848843-4-armbru@redhat.com>
    [Tweak prose as per advice from Eric]

Please note "whenever practical".  Deviating from the rule is okay when
sticking to the rule is not practical.  In other words, judgement call.

When something is used a lot, consistency matters, because it reduces
cognitive load.  The assumption "this works just like it does all over
the place" is fair, so it better be correct.

Ease of use matters, too.  When sticking to the rule leads to awkward
code, we should stop and think.  Should we deviate from the rule?  Or
can we avoid that by tweaking the interface?

Philippe's proposed interface sticks to the rule.

Another interface that does: return -1 for error, 0 for module not found
(no error), and 1 for loaded.

I think I'd prefer Philippe's.

> 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 */

This is wrong.  You need to test *errp.

Which is also wrong.  You need ERRP_GUARD().  error.h's big comment
again:

 * Call a function, receive an error from it, and pass it to the caller
 * - when the function returns a value that indicates failure, say
 *   false:
 *     if (!foo(arg, errp)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     ERRP_GUARD();
 *     foo(arg, errp);
 *     if (*errp) {
 *         handle the error...
 *     }
 * More on ERRP_GUARD() below.
 *
 * Code predating ERRP_GUARD() still exists, and looks like this:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err); // deprecated
 *     }
 * Avoid in new code.  Do *not* "optimize" it to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL without the ERRP_GUARD() guard.
 *
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.
[...]
 * = Why, when and how to use ERRP_GUARD() =
 *
 * Without ERRP_GUARD(), use of the @errp parameter is restricted:
 * - It must not be dereferenced, because it may be null.
 * - It should not be passed to error_prepend() or
 *   error_append_hint(), because that doesn't work with &error_fatal.
 * ERRP_GUARD() lifts these restrictions.
 *
 * To use ERRP_GUARD(), add it right at the beginning of the function.
 * @errp can then be used without worrying about the argument being
 * NULL or &error_fatal.
 *
 * Using it when it's not needed is safe, but please avoid cluttering
 * the source with useless code.

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

Tall order.

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

Loadable modules look grafted on, and the error handling seems quite
bad.