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

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> 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("....");

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.