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

[Lluís Vilanova]

Thomas Huth writes:

> On 27.01.2016 20:20, Lluís Vilanova wrote:
>> Lluís Vilanova writes:
>> 
>>> Thomas Huth writes:
>>>> 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 <address@hidden>
>>>>>>>> ---
>>>>>>>> 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?
>> 
>>> Well, it just seemed wrong to me using error_report() to report "regular
>>> messages" :)
>> 
>> 
>>>>> // 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?
>> 
>>> I can do that. But then should 'error_fatal' and 'error_abort' be officially
>>> deprecated in favour of error_report_fatal() and error_report_abort()?
>> 
>> Sorry, I see this is misleading. I mean deprecate directly using
>> "error_setg(error_fatal)"; you can still decide to pass error_fatal as an 
>> error
>> object to other user functions.

> Since we hardly got any of these in the code right now, I don't see an
> urgent need to explicitely say that this should be deprecated. I hope
> that people rather will use the new functions automatically instead
> since these sounds much more intuitive, IMHO.

Got it. I'm writing some of them now.

Cheers,
  Lluis