Re: [Qemu-devel] [PATCH v6 1/5] util: Introduce error reporting functions with fatal/abort

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> On 03.02.2016 10:48, Markus Armbruster wrote:
>> David Gibson <address@hidden> writes:
>> 
>>> On Tue, Feb 02, 2016 at 10:47:35PM +0100, Thomas Huth wrote:
>>>> On 02.02.2016 19:53, Markus Armbruster wrote:
>>>>> Lluís Vilanova <address@hidden> writes:
>>>> ...
>>>>
>>>>>> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
>>>>>> index 7ab2355..6c2f142 100644
>>>>>> --- a/include/qemu/error-report.h
>>>>>> +++ b/include/qemu/error-report.h
>>>>>> @@ -43,4 +43,23 @@ void error_report(const char *fmt, ...) 
>>>>>> GCC_FMT_ATTR(1, 2);
>>>>>>  const char *error_get_progname(void);
>>>>>>  extern bool enable_timestamp_msg;
>>>>>>  
>>>>>> +/* Report message and exit with error */
>>>>>> +void QEMU_NORETURN error_vreport_fatal(const char *fmt, va_list ap) 
>>>>>> GCC_FMT_ATTR(1, 0);
>>>>>> +void QEMU_NORETURN error_report_fatal(const char *fmt, ...) 
>>>>>> GCC_FMT_ATTR(1, 2);
>>>>>
>>>>> This lets people write things like
>>>>>
>>>>>     error_report_fatal("The sky is falling");
>>>>>
>>>>> instead of
>>>>>
>>>>>     error_report("The sky is falling");
>>>>>     exit(1);
>>>>>
>>>>> or
>>>>>
>>>>>     fprintf(stderr, "The sky is falling\n");
>>>>>     exit(1);
>>>>>
>>>>> I don't think that's an improvement in clarity.
>>>>
>>>> The problem is not the existing code, but that in a couple of new
>>>> patches, I've now already seen that people are trying to use
>>>>
>>>>      error_setg(&error_fatal, ... );
>>>
>>> So, I don't actually see any real advantage to error_report_fatal(...)
>>> over error_setg(&error_fatal, ...).
>> 
>> I do.  Compare:
>> 
>> (a) error_report(...);
>>     exit(1);
>> 
>> (b) error_report_fatal(...);
>> 
>> (c) error_setg(&error_fatal, ...);
>> 
>> In my opinion, (a) is clearest: even a relatively clueless reader will
>> know what exit(1) does, can guess what error_report() approximately
>> does, and doesn't need to know what it does exactly.  (b) is slightly
>> less obvious, and (c) is positively opaque.
>> 
>> Let's stick to the obvious (a) and be done with it.
>
> Ok, (a) is fine for me too, as long as we avoid (c). Lluís, could you
> maybe add that information to your patch that updates the HACKING text?

I feel such detailed advice belings into error.h.  Sketch appended.

If that doesn't succeed in keeping (c) out, make checkpatch flag it.

> (and sorry for the fuzz with error_report_fatal() ... I thought it would
> be a good solution to avoid (c), but if (a) is preferred instead, then
> we should go with that solution instead).
>
> And, by the way, what about the spots that currently already use
> error_setg(&error_abort, ....) ? Should they be turned into
> error_report() + abort() instead? Or only abort(), without error
> message, since abort() is only about programming errors?

As I wrote in my first reply to this thread, I'd like them to be cleaned
up to just abort() or assert().

I like assert(), because it gives me exactly what I can use to debug the
programming error: a core dump (if enabled) and a source location
(useful when no core dump).  I never bought the argument that we should
use abort() instead of assert(0) because "what if NDEBUG?!?".  If you
define NDEBUG, our 600+ abort()s won't save you from our 4000+
assert()s.



diff --git a/include/qapi/error.h b/include/qapi/error.h
index 45d6c72..ea7e74f 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -162,6 +162,9 @@ ErrorClass error_get_class(const Error *err);
  * human-readable error message is made from printf-style @fmt, ...
  * The resulting message should be a single phrase, with no newline or
  * trailing punctuation.
+ * 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().
  */
 #define error_setg(errp, fmt, ...)                              \
     error_setg_internal((errp), __FILE__, __LINE__, __func__,   \
@@ -213,6 +216,8 @@ void error_setg_win32_internal(Error **errp,
  * the error object.
  * Else, move the error object from @local_err to address@hidden
  * On return, @local_err is invalid.
+ * Please don't error_propagate(&error_fatal, ...), use
+ * error_report_err() and exit(), because that's more obvious.
  */
 void error_propagate(Error **dst_errp, Error *local_err);
 
@@ -291,12 +296,14 @@ void error_set_internal(Error **errp,
     GCC_FMT_ATTR(6, 7);
 
 /*
- * Pass to error_setg() & friends to abort() on error.
+ * Special error destination to abort on error.
+ * See error_setg() and error_propagate() for details.
  */
 extern Error *error_abort;
 
 /*
- * Pass to error_setg() & friends to exit(1) on error.
+ * Special error destination to exit(1) on error.
+ * See error_setg() and error_propagate() for details.
  */
 extern Error *error_fatal;