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

[Eric Blake]

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