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

[Eric Blake]

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 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.

> 
> * 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.

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature