Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/3] error: Remove unnecessary local_err variables

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/13/2016 09:52 AM, Eduardo Habkost wrote:
>
>>>
>>> There is an (ugly) difference between
>>>
>>>     error_setg(&local_err, ...);
>>>     error_propagate(errp, &local_err);
>>>
>>> and
>>>
>>>     error_setg(errp, ...);
>>>
>>> The latter aborts when @errp already contains an error, the former does
>>> nothing.
>> 
>> Why the difference? Couldn't we change that so that both are equivalent?
>
> Maybe, but I think it weakens our position. An abort() on an attempt to
> incorrectly set an error twice helps catch errors where we are throwing
> away a more useful first error message.  The documentation for
> error_propagate() already mentioned that it was an exception to the rule.

For what it's worth, both g_set_error(err, ...) and
g_propagate_error(err, ...) require !err || !*err.  To accumulate
multiple errors so that the first one wins, you have to do something
like

    if (local_err) {
        g_clear_error(errp);
        g_propagate_error(errp, local_err);
    }

error.h happend before we got GLib.  Its designers chose to deviate from
GLib and made error_propagate() accumulate errors.  This trades the
ability to detect misuse for less boilerplate:

    error_propagate(errp, local_err);

Tightening error_propagate() now would lead to a rather tedious hunt for
callers that rely on its accumulating behavior.

We could do it incrementally by renaming error_propagate() to
error_accumulate(), and have a new error_propagate() that's consistent
with error_setg().  Not sure it's worth it.

Loosening error_setg() & friends is also possible, but we'd detect fewer
misuse, and we'd be left with some superfluous code to clean up.  Not
sure that's smart.

>>> Your transformation has the error_setg() or similar hidden in F2().  It
>>> can add aborts.
>>>
>>> I think it can be salvaged: we know that @errp must not contain an error
>>> on function entry.  If @errp doesn't occur elsewhere in this function,
>>> it cannot pick up an error on the way to the transformed spot.  Can you
>>> add that to your when constraints?
>> 
>> Do we really know that *errp is NULL on entry? Aren't we allowed to call
>> functions with a non-NULL *errp?
>
> Except for error_propagate(), no.

By convention, all functions taking an Error **errp argument expect one
that can be safely passed to error_setg().  That means !errp || errp ==
&error_abort || errp == &error_fatal || !*errp.

>> 
>> See, e.g.:
>> 
>> void qmp_guest_suspend_disk(Error **errp)
>> {
>>     Error *local_err = NULL;
>>     GuestSuspendMode *mode = g_new(GuestSuspendMode, 1);
>> 
>>     *mode = GUEST_SUSPEND_MODE_DISK;
>>     check_suspend_mode(*mode, &local_err);
>>     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>     execute_async(do_suspend, mode, &local_err);
>
> That usage is a bug.  A Coccinelle script to root out such buggy
> instances would be nice.

We've discussed this before.  See for instance commit 297a364:

    qapi: Replace uncommon use of the error API by the common one
    
    We commonly use the error API like this:
    
        err = NULL;
        foo(..., &err);
        if (err) {
            goto out;
        }
        bar(..., &err);
    
    Every error source is checked separately.  The second function is only
    called when the first one succeeds.  Both functions are free to pass
    their argument to error_set().  Because error_set() asserts no error
    has been set, this effectively means they must not be called with an
    error set.
    
    The qapi-generated code uses the error API differently:
    
        // *errp was initialized to NULL somewhere up the call chain
        frob(..., errp);
        gnat(..., errp);
    
    Errors accumulate in *errp: first error wins, subsequent errors get
    dropped.  To make this work, the second function does nothing when
    called with an error set.  Requires non-null errp, or else the second
    function can't see the first one fail.
    
    This usage has also bled into visitor tests, and two device model
    object property getters rtc_get_date() and balloon_stats_get_all().
    
    With the "accumulate" technique, you need fewer error checks in
    callers, and buy that with an error check in every callee.  Can be
    nice.
    
    However, mixing the two techniques is confusing.  You can't use the
    "accumulate" technique with functions designed for the "check
    separately" technique.  You can use the "check separately" technique
    with functions designed for the "accumulate" technique, but then
    error_set() can't catch you setting an error more than once.
    
    Standardize on the "check separately" technique for now, because it's
    overwhelmingly prevalent.