diff mbox

[v4,5/5] doc: Introduce coding style for errors

Message ID 145401730295.31479.7043845714018218355.stgit@localhost (mailing list archive)
State New, archived
Headers show

Commit Message

Lluís Vilanova Jan. 28, 2016, 9:41 p.m. UTC
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(+)

Comments

Eric Blake Jan. 28, 2016, 9:50 p.m. UTC | #1
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".

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.

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

> +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/
Lluís Vilanova Jan. 28, 2016, 10:14 p.m. UTC | #2
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
diff mbox

Patch

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