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

[Lluís Vilanova]

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, ...)

  // 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?


Cheers,
  Lluis