"Daniel P. Berrange" <berra...@redhat.com> writes: > 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("....");
Or, if we decide we don't want to fuse error_report() and exit(), into error_report("...."); exit(1); > 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. I prefer to get rid of bad examples, even when they're closer to what we believe the code will look like some day, because bad examples get copied.