Re: [Qemu-devel] [PATCH v4 0/4] Introduce error_report_{fatal|abort}

[Daniel P. Berrange]

On Fri, Sep 09, 2016 at 07:05:04PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > v4 changes:
> > - remove two standard headers since they are included in osdep.h
> >   already [Fam]
> > - make sure it passes build on all platforms (no --target-list
> >   specified during configure)
> >
> > v3 changes:
> > - implement error_report_fatal using function [Markus]
> > - provide error_report_abort as well in seperate patch [Markus, Fam]
> >
> > We have many use cases that first print some error messages, then
> > quit (by either exit() or abort()). This series introduce two helper
> > functions for that.
> >
> > The old formats are mostly one of the following:
> >
> > Case one:
> >
> >   error_report(...);
> >   exit(1|EXIT_FAILURE) | abort();
> >
> > Case two:
> >
> >   error_setg(&error_{fatal|abort}, ...);
> >
> > And we can convert either of the above cases into:
> >
> >   error_report_{fatal|abort}(...);
> >
> > Two coccinelle scripts are created to help automate the work, plus
> > some manual tweaks:
> >
> > 1. very long strings, fix for over-80-chars issues, to make sure it
> >    passes checkpatch.pl.
> >
> > 2. add "return XXX" for some non-void retcode functions.
> >
> > The first two patches introduce the functions. The latter two apply
> > them.
> 
> You effectively propose to revise this coding rule from error.h:
> 
>  * Please don't error_setg(&error_fatal, ...), use error_report() and
>  * exit(), because that's more obvious.
>  * Likewise, don't error_setg(&error_abort, ...), use assert().
> 
> If we accept your proposal, you get to add a patch to update the rule :)
> 
> We've discussed the preferred way to report fatal errors to the human
> user before.  With actual patches, we can see how a change of rules
> changes the code.  Do we like the change shown by this patch set?
> 
> I believe there are a number of separate issues to discuss here:
> 
> * Shall we get rid of error_setg(&error_fatal, ...)?
> 
>   This is a no-brainer for me.  Such a simple thing should be done in
>   one way, not two ways.  I count 14 instances of
>   error_setg(&error_fatal, ...), but more than 300 of error_report(...);
>   exit(1).

NB, arguably 99% of the usage of error_setg(&error_fatal) are in
fact cases where code ought to be eventually converted to accept
an "Error **errp" parameter and let the caller decide whether to
exit or not.

IOW, if we take this approach today we'll change

   error_setg(&error_fatal, "....");

into

   error_report_fatal("....");

and then later potentially change it back again to

   error_setg(errp, "....");


This isn't the end of the world, but I'm just not convinced that
the intermediate change to error_report_fatal() buys us anything
positive overall.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|