Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors

[Thomas Huth <thuth@redhat.com>]

On 20.01.2016 15:10, Lluís Vilanova wrote:
> Thomas Huth writes:
> 
>> On 18.01.2016 21:26, Eric Blake wrote:
>>> On 01/15/2016 06:54 AM, Lluís Vilanova wrote:
>>>> Gives some general guidelines for reporting errors in QEMU.
>>>>
>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> ---
>>>> HACKING |   36 ++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 36 insertions(+)
>> ...
>>>> +Functions in this header are used to accumulate error messages in an 'Error'
>>>> +object, which can be propagated up the call chain where it is finally reported.
>>>> +
>>>> +In its simplest form, you can immediately report an error with:
>>>> +
>>>> +    error_setg(&error_fatal, "Error with %s", "arguments");
>>>
>>> This paradigm doesn't appear anywhere in the current code base
>>> (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but
>>> nothing directly passes error_fatal).  It's a bit odd to document
>>> something that isn't actually used.
> 
>> +1 for _not_ documenting this here: IMHO this looks ugly. If we want
>> something like this, I think we should introduce a proper
>> error_report_fatal() function instead.
> 
> That's a bit of a chicken and egg problem. My main intention was to provide a
> best practices summary on reporting messages/errors, since QEMU's code is really
> heterogeneous on that regard. But there seems to be no consensus on some details
> of what the proper way should be with the current interfaces.
> 
> Utility functions for "regular messages", warnings, fatals and aborts would
> definitiely be an improvement IMHO, but I dont have time to adapt existing code
> to these (and I was told not to add unused utility functions for this).
> 
> Now, if I were able to add such functions, it'd be something like:
> 
>   // Generate message "as is"; not sure if this should exist.
>   message_report(fmt, ...)

Not sure what this should be good for? We've already got error_report()
that generates messages "as is", haven't we?

>   // Generate message with prepended file/line information for the caller.
>   // Calls exit/abort on the last two.
>   error_report_{warn,fatal,abort}(fmt, ...)
> 
>   // Same with an added message from strerror.
>   error_report_{warn,fatal,abort}_errno(fmt, ...)
> 
> But, should I add these without providing extensive patches that refactor code
> to use them?

Maybe create a patch that introduces the _fatal and _abort functions
(I'd skip the _warn functions for now), and use them in one or two files
(e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should
not be that much of work, and could be a good base for further discussion?

 Thomas