Thomas Huth <th...@redhat.com> writes:
> On 03.02.2016 10:48, Markus Armbruster wrote:
>> David Gibson <da...@gibson.dropbear.id.au> 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 <vilan...@ac.upc.edu> 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 *@dst_errp.
* 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;
