Eric Blake <ebl...@redhat.com> 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.