Re: Questionable aspects of QEMU Error's design

[BALATON Zoltan]

On Thu, 2 Apr 2020, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> > 02.04.2020 12:36, BALATON Zoltan wrote:
> > > On Thu, 2 Apr 2020, Vladimir Sementsov-Ogievskiy wrote:
> > > > 01.04.2020 23:15, Peter Maydell wrote:
> > > > > On Wed, 1 Apr 2020 at 10:03, Markus Armbruster <address@hidden> wrote:
> > > > > > QEMU's Error was patterned after GLib's GError.  Differences include:
> > > > >
> > > > >  From my POV the major problem with Error as we have it today
> > > > > is that it makes the simple process of writing code like
> > > > > device realize functions horrifically boilerplate heavy;
> > > > > for instance this is from hw/arm/armsse.c:
> > > > >
> > > > >          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
> > > > >                                   "memory", &err);
> > > > >          if (err) {
> > > > >              error_propagate(errp, err);
> > > > >              return;
> > > > >          }
> > > > >          object_property_set_link(cpuobj, OBJECT(s), "idau", &err);
> > > > >          if (err) {
> > > > >              error_propagate(errp, err);
> > > > >              return;
> > > > >          }
> > > > >          object_property_set_bool(cpuobj, true, "realized", &err);
> > > > >          if (err) {
> > > > >              error_propagate(errp, err);
> > > > >              return;
> > > > >          }
> > > > >
> > > > > 16 lines of code just to set 2 properties on an object
> > > > > and realize it. It's a lot of boilerplate and as
> > > > > a result we frequently get it wrong or take shortcuts
> > > > > (eg forgetting the error-handling entirely, calling
> > > > > error_propagate just once for a whole sequence of
> > > > > calls, taking the lazy approach and using err_abort
> > > > > or err_fatal when we ought really to be propagating
> > > > > an error, etc). I haven't looked at 'auto propagation'
> > > > > yet, hopefully it will help?
> > > >
> > > > Yes, after it the code above will look like this:
> > > >
> > > > ... some_func(..., errp)
> > > > {
> > > >    ERRP_AUTO_PROPAGATE(); # magic macro at function start, and no "Error *err" 
> > > > definition
> > > >
> > > > ...
> > > >          object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
> > > >                                   "memory", errp);
> > > >          if (*errp) {
> > > >              return;
> > > >          }
> > > >          object_property_set_link(cpuobj, OBJECT(s), "idau", errp);
> > > >          if (*errp) {
> > > >              return;
> > > >          }
> > > >          object_property_set_bool(cpuobj, true, "realized", errp);
> > > >          if (*errp) {
> > > >              return;
> > > >          }
> > > > ...
> > > > }
> > > >
> > > > - propagation is automatic, errp is used directly and may be safely 
> > > > dereferenced.
> > >
> > > Not much better. Could it be something like:
> >
> > Actually, much better, as it solves some real problems around error propagation.
>
> The auto propagation patches' stated aim is to fix &error_fatal not to
> eat hints, and to provide more useful stack backtraces with
> &error_abort.  The slight shrinking of boilerplate is a welcome bonus.
>
> For a bigger improvement, have the functions return a useful value, as
> discussed elsewhere in this thread.
>
> > >      ERRP_RET(object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
> > >                                        "memory", errp));
> > >      ERRP_RET(object_property_set_link(cpuobj, OBJECT(s), "idau", errp));
> > >      ERRP_RET(object_property_set_bool(cpuobj, true, "realized", errp));
> >
> > and turn all
> >
> > ret = func(...);
> > if (ret < 0) {
> >     return ret;
> > }
> >
> > into
> >
> > FAIL_RET(func(...))
> >
> > ?
> >
> > Not a problem to make such macro.. But I think it's a bad idea to turn all the 
> > code
> > into sequence of macro invocations. It's hard to debug and follow.
>
> Yes.  Hiding control flow in macros is almost always too much magic.
> There are exceptions, but this doesn't look like one.

I did't like this idea of mine too much either so I agree but I see no 
other easy way to simplify this. If you propose changing function return 
values maybe these should return errp instead of passing it as a func 
parameter? Could that result in simpler code and less macro magic needed?

Regards,
BALATON Zoltan