Re: [PATCH v6] hw/core/qdev: cleanup Error ** variables

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 12/5/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>
>>>> @@ -918,27 +917,26 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>>>           }
>>>>           } else if (!value && dev->realized) {
>>>> -        Error **local_errp = NULL;
>>>> +        /* We want local_err to track only the first error */
>>>>            QLIST_FOREACH(bus, &dev->child_bus, sibling) {
>>>> -            local_errp = local_err ? NULL : &local_err;
>>>>                object_property_set_bool(OBJECT(bus), false, "realized",
>>>> -                                     local_errp);
>>>> +                                     local_err ? NULL : &local_err);
>>>>            }
>>>
>>> This is a rather unusual way to keep the first error of several.
>
> It may be unusual, but has the benefit of avoiding error_propagate...

Non-issue if the error_propagate() gets replaced by
ERRP_AUTO_PROPAGATE(), isn't it?

>>> qapi/error.h advises:
>>>
>>>    * Receive and accumulate multiple errors (first one wins):
>>>    *     Error *err = NULL, *local_err = NULL;
>>>    *     foo(arg, &err);
>>>    *     bar(arg, &local_err);
>>>    *     error_propagate(&err, local_err);
>>>    *     if (err) {
>>>    *         handle the error...
>>>    *     }
>>
>> Hmm, honestly, I like more what I've written:
>>
>> 1. less code
>> 2. logic is more clean: we store first error to local_err, and after first error
>>      pass NULL as a parameter. No propagation or extra error variables.
>> 3. more efficient (no propagation, no extra allocation for errors which we'll drop
>>      anyway) (I understand that efficiency of error path is not thing to care about,
>>      so it's at third place)
>>
>> Also, propagation which you propose is also unusual thing (it proposed in comment,
>> but who reads it :). I've never seen it before, and I've to go and check that
>> error_propagate works correctly when first argument is already set.

When you think you can improve on the common, documented pattern, you're
invited to update the documentation and the existing uses of the
pattern.

If everybody "improved" on common, documented patterns locally, the code
would become needlessly hard to read for developers with experience in
the pattern's area.

>> So, I'd prefer to keep now this patch as is, and to convert later if we really need it.

I want this to match the common, documented pattern.  Whether we make it
match before or after your ERRP_AUTO_PROPAGATE() work doesn't matter to
me.

>>> If replacing this by the usual way is too troublesome now, we can do it
>>> after the ERRP_AUTO_PROPAGATE() conversion.  Your choice.
>
> ...and after conversion to use ERRP_AUTO_PROPATATE(), the use of
> error_propagate() should NOT occur in any code _except_ for the macro
> definition (any other use of the function points out a place where we
> failed to use the macro to get rid of boilerplate).

I figure we still need it in the (rare) cases where we want to ignore
some of a function's errors, as we do in fit_load_fdt().  If that
bothers us, we can try to find a solution that avoids the boilerplate.