On Fri, Sep 09, 2016 at 07:05:04PM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> 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 :|
