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