Re: [PATCH v12 16/16] machine: Make smp_parse return a boolean

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 01/10/21 19:15, Daniel P. Berrangé wrote:
>> On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote:
>>> On 29/09/21 04:58, Yanan Wang wrote:
>>>> @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, 
>>>> const char *name,
>>>>            return;
>>>>        }
>>>> -    smp_parse(ms, config, errp);
>>>> -    if (*errp) {
>>>> +    if (!smp_parse(ms, config, errp)) {
>>>>            qapi_free_SMPConfiguration(config);
>>>>        }
>>>>    }
>>>>
>>>
>>> This is actually a leak, so I'm replacing this patch with
>> 
>> This patch isn't adding a leak, as there's no change in
>> control flow / exit paths.  AFAICT, the leak was introduced
>> in patch 15 instead, so the code below shoudl be squashed
>> into that, and this patch left as-is.
>
> Yes, even better make it a separate patch and fix the conflict in patch
> 15.  But I'm still not sure of the wisdom of this patch.
>
> At this point smp_parse has exactly one caller and it doesn't care about
> the return value.  The "return a boolean" rule adds some complexity (and
> a possibility for things to be wrong/inconsistent) to the function for
> the benefit of the callers.

Yes, but returning something is only a minor burden.  It also makes
success vs. failure obvious at a glance.

I'm not worrying about inconsistency anymore.  In a way, void functions
are an exception.  Many non-void functions return a distinct error value
on failure, like NULL.  The only kind of inconsistency I can remember
seeing in these functions is forgetting to set an error.  Can be screwed
up in a void function just as easily.

>                              If there is only one caller, as is the case
> here or for virtual functions, the benefit can well be zero (this case)
> or negative (virtual functions).

Two small benefits here:

1. No need for ERRP_GUARD().

2. Conforms to the rules.  Rules are not laws, but let's stick to them
when it's as easy as it is here.

For what it's worth, GLib always advised users of GError to return a
value.  We chose to deviate for our Error, then spent nine years
learning how that leads to cumbersome code, leading us 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.
    
    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]