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

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <address@hidden> writes:
>> Gives some general guidelines for reporting errors in QEMU.
>> 
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---
>> HACKING |   37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>> 
>> diff --git a/HACKING b/HACKING
>> index 12fbc8a..b738bce 100644
>> --- a/HACKING
>> +++ b/HACKING
>> @@ -157,3 +157,40 @@ 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()').

> The "do not use exit() or abort()" part applies only if we adopt your
> new error reporting functions.

>> +
>> +As a general rule, use the 'fatal' forms below for errors that can be 
>> triggered

> Where's "below"?

It's the following two sections, sorry.


>> +by the user (e.g., commandline, hotplug, etc.), and the 'abort' forms for
>> +programming errors that the user should not be able to trigger.

> This paragraph applies only if we adopt your new error reporting
> functions.

>> +
>> +7.1. Simple error messages
>> +
>> +The 'error_report*()' functions in "include/qemu/error-report.h" will
>> +immediately report error messages to the user.

> Suggest "the human 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 
>> unimplemented
>> +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()'.

> This paragraph applies only if we adopt your new error reporting
> functions.  Without them: "Do *not* exit() or abort() on errors
> that can be triggered...", and scratch the last sentence.

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

> This is probably too terse to help much on its own.  Perhaps
> error-report.h should have usage information, like error.h.

I can try adding that, although I've barely used this part of the interface.


>> +
>> +7.3. More complex error management
>> +
>> +The functions in "include/qapi/error.h" can be used to accumulate error 
>> messages

> Long line.

>> +in an 'Error' object, which can be propagated up the call chain where it is
>> +finally reported.

> "Accumulate" suggests accumulating multiple error messages, and that's
> not quite right.  Perhaps: "to capture an error message".

> "where it is finally reported" is inaccurate.  It may or may not be
> reported.  Perhaps: "until it is handled, and possibly reported to the
> user".

>> +
>> +WARNING: The special 'error_fatal' and 'error_abort' objects follow the same
>> +constraints as the 'error_report_fatal()' and 'error_report_abort()' 
>> functions.

> Without your new error reporting functions: "Do *not* use &error_fatal
> and &error_abort to handle errors that can be triggered by guest code,
> just like exit() and abort()."

> Suggest to explicitly point to the big usage comment in error.h.


Thanks,
  Lluis