Re: [Qemu-devel] [PATCH v4 5/5] doc: Introduce coding style for errors

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Eric Blake writes:

> On 01/28/2016 02:41 PM, Lluís Vilanova wrote:
>> Gives some general guidelines for reporting errors in QEMU.
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> HACKING |   33 +++++++++++++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> 
>> diff --git a/HACKING b/HACKING
>> index 12fbc8a..f5783d4 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -157,3 +157,36 @@ painful. These are:
>> * you may assume that integers are 2s complement representation
>> * you may assume that right shift of a signed integer duplicates
>> the sign bit (ie it is an arithmetic shift, not a logical shift)
>> +
>> +7. Error reporting
>> +
>> +QEMU provides various mechanisms for reporting errors using a uniform format,
>> +ensuring the user will receive them (e.g., shown in QMP when necessary). You
>> +should use one of these mechanisms instead of manually reporting them (i.e., do
>> +not use 'printf()', 'exit()' or 'abort()').

> abort() for unreachable code may be okay, but I'm not sure how to word
> that.  Maybe "avoid use 'printf()' or 'exit()', and minimize use of
> 'abort()' situations that should be unreachable code".

Hmmm. I was thinking it'd be more informative to always use error_report_abort()
instead of abort:

* The program name is shown (great for multi-app logs)
* The aborting location is shown (a bit useful to quickly see where it comes
  from)
* A message can be provided

I think the message should be optional, since error_report_abort() already
provides more information than plain abort().

But if that seems unreasonable, I can reword it as:

  QEMU provides various mechanisms for reporting errors using a uniform format,
  ensuring the user will receive them (e.g., shown in QMP when necessary). You
  should use one of these mechanisms instead of manually reporting them; i.e.,
  do not use 'printf()' nor 'exit()', and minimize the use of 'abort()' to
  situations where code should be unreachable and an error message does not make
  sense.


> May be worth mentioning that if the user can trigger it (command line,
> hotplug, etc) then we want fatal; if it represents a programming bug
> that the user should not be able to trigger, then abort is okay.

So true. I'll add these.


>> +
>> +7.1. Simple error messages
>> +
>> +The 'error_report*()' functions in "include/qemu/error-report.h" will
>> +immediately report error messages to the user.
>> +
>> +WARNING: Do *not* use 'error_report_fatal()' or 'error_report_abort()' for
>> +errors that are (or can be) triggered by guest code (e.g., some unimplimented

> s/unimplimented/unimplemented/

Fixed in v5 (sorry I forgot about it).


>> +corner case in guest code translation or device code). Otherwise that can be
>> +abused by guest code to terminate QEMU. Instead, you should use
>> +'error_report()'.
>> +
>> +7.2. Errors in user inputs
>> +
>> +The 'loc_*()' functions in "include/qemu/error-report.h" will extend the
>> +messages from 'error_report*()' with references to locations in inputs provided
>> +by the user (e.g., command line arguments or configuration files).
>> +
>> +7.3. More complex error management
>> +
>> +The functions in "include/qapi/error.h" can be used to accumulate error messages
>> +in an 'Error' object, which can be propagated up the call chain where it is
>> +finally reported.
>> +
>> +WARNING: The special 'error_fatal' and 'error_abort' objects follow the same
>> +constrains as the 'error_report_fatal' and 'error_report_abort' functions.

> s/constrains/constraints/

Will add.


Thanks,
  Lluis