Re: Questionable aspects of QEMU Error's design

[Eduardo Habkost]

On Sat, Apr 04, 2020 at 12:59:27PM +0200, Markus Armbruster wrote:
> Markus Armbruster <address@hidden> writes:
> 
> > Markus Armbruster <address@hidden> writes:
> >
> >> QEMU's Error was patterned after GLib's GError.  Differences include:
> > [...]
> >> * Return value conventions
> >>
> >>   Common: non-void functions return a distinct error value on failure
> >>   when such a value can be defined.  Patterns:
> >>
> >>   - Functions returning non-null pointers on success return null pointer
> >>     on failure.
> >>
> >>   - Functions returning non-negative integers on success return a
> >>     negative error code on failure.
> >>
> >>   Different: GLib discourages void functions, because these lead to
> >>   awkward error checking code.  We have tons of them, and tons of
> >>   awkward error checking code:
> >>
> >>     Error *err = NULL;
> >>     frobnicate(arg, &err);
> >>     if (err) {
> >>         ... recover ...
> >>         error_propagate(errp, err);
> >>     }
> >>
> >>   instead of
> >>
> >>     if (!frobnicate(arg, errp))
> >>         ... recover ...
> >>     }
> >>
> >>   Can also lead to pointless creation of Error objects.
> >>
> >>   I consider this a design mistake.  Can we still fix it?  We have more
> >>   than 2000 void functions taking an Error ** parameter...
> >>
> >>   Transforming code that receives and checks for errors with Coccinelle
> >>   shouldn't be hard.  Transforming code that returns errors seems more
> >>   difficult.  We need to transform explicit and implicit return to
> >>   either return true or return false, depending on what we did to the
> >>   @errp parameter on the way to the return.  Hmm.
> > [...]
> >
> > To figure out what functions with an Error ** parameter return, I used
> > Coccinelle to find such function definitions and print the return types.
> > Summary of results:
> >
> >    2155 void
> >     873 signed integer
> >     494 pointer
> >     153 bool
> >      33 unsigned integer
> >       6 enum
> >    ---------------------
> >    3714 total
> >
> > I then used Coccinelle to find checked calls of void functions (passing
> > &error_fatal or &error_abort is not considered "checking" here).  These
> > calls become simpler if we make the functions return a useful value.  I
> > found a bit under 600 direct calls, and some 50 indirect calls.
> >
> > Most frequent direct calls:
> >
> >     127 object_property_set_bool
> >      27 qemu_opts_absorb_qdict
> >      16 visit_type_str
> >      14 visit_type_int
> >      10 visit_type_uint32
> >
> > Let's have a closer look at object_property_set() & friends.  Out of
> > almost 1000 calls, some 150 are checked.  While I'm sure many of the
> > unchecked calls can't actually fail, I am concerned some unchecked calls
> > can.
> >
> > If we adopt the convention to return a value that indicates success /
> > failure, we should consider converting object.h to it sooner rather than
> > later.
> >
> > Please understand these are rough numbers from quick & dirty scripts.
> 
> Paolo, Daniel, Eduardo,
> 
> Please pick one for QOM:

Replying this without reading the whole discussion and context:

> 
> * Do nothing.  Looks like
> 
>      object_property_set_bool(..., &err);
>      if (err) {
>          error_propagate(errp, err);
>          return;
>      }
> 
> * Return true on success, false on error.  Looks like
> 

I prefer this one.

>      if (!object_property_set_bool(..., errp)) {
>          return;
>      }
> 
> * Return 0 on success, -1 on error.  Looks like
> 
>      if (object_property_set_bool(..., errp) < 0) {
>          return;
>      }
> 
>   This is slightly more likely to require line wrapping than the
>   previous one.

-- 
Eduardo