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

[Markus Armbruster]

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

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

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