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

[Vladimir Sementsov-Ogievskiy]

06.12.2019 11:26, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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.

That's reasonable, I understand.

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

Then, let's after.

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


-- 
Best regards,
Vladimir