On Sat, Apr 04, 2020 at 12:59:27PM +0200, Markus Armbruster wrote:
> Markus Armbruster <arm...@redhat.com> writes:
>
> > Markus Armbruster <arm...@redhat.com> 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
