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

[Markus Armbruster]

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

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

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

Opinions?