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

[Eric Blake <eblake@redhat.com>]

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

>> 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.
> 
> So, I'd prefer to keep now this patch as is, and to convert later if we really need it.
> 
>>
>> 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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org