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

[Thomas Huth <thuth@redhat.com>]

On 03.02.2016 10:48, Markus Armbruster wrote:
> David Gibson <david@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 <vilanova@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?
(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?

 Thomas