diff mbox

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

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

Commit Message

Lluís Vilanova Feb. 2, 2016, 4:14 p.m. UTC
Gives some general guidelines for reporting errors in QEMU.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 HACKING |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Comments

Markus Armbruster Feb. 2, 2016, 7:10 p.m. UTC | #1
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Gives some general guidelines for reporting errors in QEMU.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  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.
Lluís Vilanova Feb. 3, 2016, 1:47 p.m. UTC | #2
Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Gives some general guidelines for reporting errors in QEMU.
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>> 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
Markus Armbruster Feb. 3, 2016, 2:41 p.m. UTC | #3
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Gives some general guidelines for reporting errors in QEMU.
>>> 
>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>> ---
>>> HACKING |   37 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 37 insertions(+)
>>> 
>>> diff --git a/HACKING b/HACKING
>>> index 12fbc8a..b738bce 100644
>>> --- a/HACKING
>>> +++ b/HACKING
[...]
>>> +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.

Documenting something you're not familiar with risks messy and laborious
review.  I'd simply drop this section for now.  If you have appetite for
more after you got the rest in, you can do another patch.

[...]
Lluís Vilanova Feb. 3, 2016, 3:17 p.m. UTC | #4
Markus Armbruster writes:

> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>> Markus Armbruster writes:
>> 
>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>> Gives some general guidelines for reporting errors in QEMU.
>>>> 
>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> ---
>>>> HACKING |   37 +++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 37 insertions(+)
>>>> 
>>>> diff --git a/HACKING b/HACKING
>>>> index 12fbc8a..b738bce 100644
>>>> --- a/HACKING
>>>> +++ b/HACKING
> [...]
>>>> +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.

> Documenting something you're not familiar with risks messy and laborious
> review.  I'd simply drop this section for now.  If you have appetite for
> more after you got the rest in, you can do another patch.

Mmmm, I still think that a terse reference is better at directing developers to
the right header than just not commenting it. I think that this type of patches
are not funny to anyone, so this risks having no metion of loc_* in the near/mid
future.

But hey, I might just be too pessimistic :)

Thanks,
  Lluis
Markus Armbruster Feb. 3, 2016, 3:53 p.m. UTC | #5
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>> Markus Armbruster writes:
>>> 
>>>> Lluís Vilanova <vilanova@ac.upc.edu> writes:
>>>>> Gives some general guidelines for reporting errors in QEMU.
>>>>> 
>>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>>> ---
>>>>> HACKING |   37 +++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 37 insertions(+)
>>>>> 
>>>>> diff --git a/HACKING b/HACKING
>>>>> index 12fbc8a..b738bce 100644
>>>>> --- a/HACKING
>>>>> +++ b/HACKING
>> [...]
>>>>> +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.
>
>> Documenting something you're not familiar with risks messy and laborious
>> review.  I'd simply drop this section for now.  If you have appetite for
>> more after you got the rest in, you can do another patch.
>
> Mmmm, I still think that a terse reference is better at directing developers to
> the right header than just not commenting it. I think that this type of patches
> are not funny to anyone, so this risks having no metion of loc_* in the near/mid
> future.
>
> But hey, I might just be too pessimistic :)

If you want to cover error locations in HACKING briefly, that's okay
with me, but it needs to be correct.  I'll look over your proposed patch
again.
Markus Armbruster Feb. 3, 2016, 4:58 p.m. UTC | #6
Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Gives some general guidelines for reporting errors in QEMU.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>

I started re-reviewing this, but my suggestions quickly became a rewrite
(sorry), so I'm just posting just that.  I'll turn it into a proper
patch you can include in your series.


7. Error handling and reporting

7.1 Reporting errors to the human user

Do not use printf(), fprintf() or monitor_printf().  Instead, use
error_report() or error_vreport() from error-report.h.  This ensures the
error is reported in the right place (current monitor or stderr), and in
a uniform format.

Use error_printf() & friends to print additional information.

error_report() prints the current location.  In certain common cases
like command line parsing, the current location is tracked
automatically.  To manipulate it manually, use the loc_*() from
error-report.h.

7.2 Propagating errors

An error can't always be reported to the user right where it's detected,
but often needs to be propagated up the call chain to a place that can
handle it.  This can be done in various ways.

The most flexible one is Error objects.  See error.h for usage
information.

Use the simplest suitable method to communicate success / failure to
callers.  Stick to common methods: non-negative on success / -1 on
error, non-negative / -errno, non-null / null, or Error objects.

Example: when a function returns a non-null pointer on success, and it
can fail only in one way (as far as the caller is concerned), returning
null on failure is just fine, and certainly simpler and a lot easier on
the eyes than propagating an Error object through an Error ** parameter.

Example: when a function's callers need to report details on failure
only the function really knows, use Error **, and set suitable errors.

Do not report an error to the user when you're also returning an error
for somebody else to handle.  Leave the reporting to the place that
consumes the error returned.

7.3 Handling errors

Calling exit() is fine when handling configuration errors during
startup.  It's problematic during normal operation.  In particular,
monitor commands should never exit().

Do not call exit() or abort() to handle an error that can be triggered
by the guest (e.g., some unimplemented corner case in guest code
translation or device emulation).  Guests should not be able to
terminate QEMU.

Note that &error_fatal is just another way to exit(1), and &error_abort
is just another way to abort().
diff mbox

Patch

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()').
+
+As a general rule, use the 'fatal' forms below for errors that can be triggered
+by the user (e.g., commandline, hotplug, etc.), and the 'abort' forms for
+programming errors that the user should not be able to trigger.
+
+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 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
+constraints as the 'error_report_fatal()' and 'error_report_abort()' functions.