Re: [Qemu-devel] [RFC PATCH v1 0/5] Add error_abort and associated cleanups

[Igor Mammedov]

On Thu, 05 Dec 2013 11:37:27 +0100
Paolo Bonzini <address@hidden> wrote:

> Il 03/12/2013 21:33, Igor Mammedov ha scritto:
> > I'm sorry for hijacking thread, but that actually an issue that started an
> > original discussion.
> > Where void returning QOM API functions are used with NULL, without any 
> > chance
> > to detect that error happened. So abusing NULL errp in this functions
> > might lead to hard to find runtime errors.
> > I think Eric's suggestion was to enforce passing non NULL errp and let 
> > caller
> > to deal with error gracefully so that above mentioned misuse was impossible.
> > Why is ignoring errors from "void foo(...)" like API considered acceptable?
> 
> See http://permalink.gmane.org/gmane.comp.emulators.qemu/243779
> 
> > * Peter's alternative
> >   + self-documenting
> >   + consistent
> >   + predictable
> 
> I'll add another small advantage which is fewer SLOC.
There is not argument against Peter's approach at all,
question is what do we do with NULL errp in void API functions?

> 
> > * make Error* mandatory for all void functions
one more advantage:
+ not need to pepper every property setter/getter with local_error + 
error_propagate(),
  i.e. reduced code duplication.

> >   + consistent
> >   + almost predictable (because in C you can ignore return values)
there is no return values from void functions.

> >   - not necessarily does the right thing (e.g. cleanup functions)
we can pass &error_abort instead of NULL there if we don't care. If there will
be error it would mean something went horribly wrong and perhaps code
should care if error happens there.

for special cases we could invent &ignore_error if there will be real need for 
it.

> >   - requires manual effort to abide to the policy
with assert inside API there is no manual effort. But as Marcus
noted these errors will be only runtime detectable :(

> 
> Better wording of the last: a missing &error_abort is easier to spot
> than a missing assert_no_error(errp).
> 
> Paolo