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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/09/2016 12:05 PM, Markus Armbruster wrote:
>> 
>> 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?
>
> That includes diffstats, to help gauge the extent of the change (not as
> easy is gauging the ratio of the changed code to the rest of the code
> that did not need to change - if we are touching 40% of all callers, it
> is invasive because the remaining 60% is not that much more dominant; if
> we are touching 2% of all callers it is a great patch for keeping us
> consistent with the remaining 98%).
>
> error_report_fatal() had a lot of hits:
>
>  76 files changed, 557 insertions(+), 799 deletions(-)
>  create mode 100644 scripts/coccinelle/error_report_fatal.cocci
>
> while error_report_abort() was not as common:
>
>  12 files changed, 41 insertions(+), 32 deletions(-)
>  create mode 100644 scripts/coccinelle/error_report_abort.cocci

I count ~1350 occurences of error_report() in master, a smilar number of
exit(), and ~650 abort().  The series fuses 329 of them into
error_report_fatal(), and 16 into error_report_abort().  Touches roughly
one in four error_report().

Additionally, it normalizes 13 uses of error_setg(&error_fatal, ...) and
2 of error_setg(&error_abort, ...).

>> 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).
>
> So this adds some of the percentages that I allude to above: 14/300 is
> definitely a case where the outliers are worth making common (so getting
> rid of error_setg(&error_fatal) makes sense).  Now, whether we get rid
> of the differences by making it all error_setg()/exit() (to match the
> 300), or whether we change ALL of these to a new error_report_fatal (for
> 314 changes) is up for a bit more debate:
>
>> 
>> * Shall we fuse error_report() and exit() into error_report_fatal()?
>> 
>>   Saves ~200 lines, not counting the Coccinelle semantic patch.
>> 
>>   I think the real question is what's easier to read and to write.  Do
>>   you prefer something like
>> 
>>                     error_report("ISA bus not available for %s", c->name);
>>                     exit(1);
>> 
>>   or something like
>> 
>>                     error_report_fatal("ISA bus not available for %s",
>>                                        c->name);
>> 
>>   The second form saves a tiny bit of instruction space, I guess.
>
> I can live with error_report_fatal().  It makes indentation a bit more
> awkward to think about, and hides the fact that it is exit()ing, but if
> done commonly enough (and 314 instances in the code base seems
> relatively common) along with a Coccinelle script to enforce it, it
> seems like it would be a usable idiom.

Roughly 1000 exit() left.

Terminating the program when you shouldn't is a fairly common issue.  To
finding places that do that, you need to grep for functions that do it.
The more we create, the more cumbersome that gets.

This isn't a particularly strong argument against having
error_report_fatal().  However, we need arguments *for* having it :)

>> * Shall we get rid of error_setg(&error_abort, ...)?
>> 
>>   Getting rid of it is again a no-brainer, but what to replace it with
>>   isn't.
>> 
>>   In my personal opinion, abort() is a perfectly fine way to handle
>>   "this cannot happen" conditions, and printing pretty messages right
>>   before abort() is a waste of time.  If the abort() happens, the
>>   program is broken, and all the end user needs to know is that he needs
>>   to find someone to debug and fix it.  If the end user really needs to
>>   know more, use of abort() is usually wrong.
>> 
>>   But others have different opinions.  If you want to print pretty
>>   messages before abort(), you get to print them.
>> 
>>   The question is whether to provide a fused error_report_abort().  I'd
>>   be willing to provide it just for symmetry with error_report_fatal(),
>>   if we decide we want error_report_fatal().
>
> I'm leaning towards error_report_abort() as useless, agreeing with the
> argument that reporting a nice message before abort()ing is a waste of
> time (the ideal program never aborts, so the nice message is either dead
> code, or the error is reachable in situations such that you should not
> have been trying to abort).  But if we don't convert
> error_report_abort(), then having JUST error_report_fatal() as shorthand
> on its own merits becomes a bit tougher sell.

Yes.

> I don't know that I have swayed any opinions, so much as just added
> commentary to the discussion.  I guess we could easily split this into a
> trivial patch (get rid of the 14 error_setg(&error_fatal) via Coccinelle
> to error_report()/exit()) as a patch worth applying now, and a second
> Coccinelle conversion of error_report()/exit() to error_report_fatal()
> as a more ambiguous change of whether we like it or not.

Yes, split this into the uncontroversial part getting rid of the
error_setg() usage that goes against the comment in error.h, and the
part we're not sure about.