Re: Questionable aspects of QEMU Error's design

[Vladimir Sementsov-Ogievskiy]

02.04.2020 8:54, Markus Armbruster wrote:
> Peter Maydell <address@hidden> writes:
>
> > 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?
>
> With that, you can have
>
>          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;
>          }
>
> but you have to add
>
>          ERRP_AUTO_PROPAGATE();
>
> right at the beginning of the function.
>
> It's a small improvement.  A bigger one is
>
>          if (object_property_set_link(cpuobj, OBJECT(&s->cpu_container[i]),
>                                       "memory", errp)) {
>              return;

return false; you mean, assuming we converting the caller too...

>          }
>          if (object_property_set_link(cpuobj, OBJECT(s), "idau", errp)) {
>              return;
>          }
>          if (object_property_set_bool(cpuobj, true, "realized", errp)) {
>              return;
>          }

Hmm..

Somehow, in general, especially with long function names and long parameter 
lists I prefer

ret = func(..);
if (ret < 0) {
    return ret;
}

notation to moving the entire function call into if condition.. Extra level of 
parentheses complicates the view of the code.

But, yes, may be for boolean functions it looks a bit strange:

ok = func(..);
if (!ok) {
    return false;
}


> This is item "Return value conventions" in the message you replied to.
>
> Elsewhere in this thread, I discussed the difficulties of automating the
> conversion to this style.  I think I know how to automate converting the
> calls to use the bool return value, but converting the functions to
> return it looks hard.  We could do that manually for a modest set of
> frequently used functions.  object.h would top my list.

Are you sure that adding a lot of boolean functions is a good idea? I somehow 
feel better with more usual int functions with -errno on failure.

Bool is a good return value for functions which are boolean by nature: checks, 
is something correspond to some criteria. But for reporting an error I'd prefer 
-errno.

--
Best regards,
Vladimir