Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors

[Markus Armbruster <armbru@redhat.com>]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Markus Armbruster writes:
>
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>> On Mon, Nov 23, 2015 at 07:41:24PM +0100, Lluís Vilanova wrote:
>>>> Gives some general guidelines for reporting errors in QEMU.
>>>> 
>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> ---
>>>> HACKING |   31 +++++++++++++++++++++++++++++++
>>>> 1 file changed, 31 insertions(+)
>>>> 
>>>> diff --git a/HACKING b/HACKING
>>>> index 12fbc8a..e59bc34 100644
>>>> --- a/HACKING
>>>> +++ b/HACKING
>>>> @@ -157,3 +157,34 @@ 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 two different mechanisms for reporting errors. You
>>>> should use one
>>>> +of these mechanisms instead of manually reporting them (i.e., do not use
>>>> +'printf', 'exit' or 'abort').
>>>> +
>>>> +7.1. Errors in user inputs
>>>> +
>>>> +QEMU provides the functions in "include/qemu/error-report.h" to
>>>> report errors
>>>> +related to inputs provided by the user (e.g., command line arguments or
>>>> +configuration files).
>>>> +
>>>> +These functions generate error messages with a uniform format
>>>> that can reference
>>>> +a location on the offending input.
>>>> +
>>>> +7.2. Other errors
>>>> +
>>>> +QEMU provides the functions in "include/qapi/error.h" to report
>>>> other types of
>>>> +errors (i.e., not triggered by command line arguments or
>>>> configuration files).
>>>> +
>>>> +Functions in this header are used to accumulate error messages in
>>>> an 'Error'
>>>> +object, which can be propagated up the call chain where it is
>>>> finally reported.
>>>> +
>>>> +In its simplest form, you can immediately report an error with:
>>>> +
>>>> +    error_setg(&error_warn, "Error with %s", "arguments");
>>>> +
>>>> +See the "include/qapi/error.h" header for additional convenience
>>>> functions and
>>>> +special arguments. Specially, see 'error_fatal' and 'error_abort'
>>>> to show errors
>>>> +and immediately terminate QEMU.
>>> 
>>> I don't think this "Errors in user inputs" vs "Other errors" distinction
>>> really makes sense. Whether an error raised in a piece of code is related
>>> to user input or not is almost impossible to determine in practice. So as
>>> a rule to follow it is not practical.
>>> 
>>> AFAIK, include/qemu/error-report.h is the historical failed experiment
>>> in structured error reporting, while  include/qapi/error.h is the new
>>> preferred error reporting system that everything should be using.
>
>> Not quite :)
>
>> error-report.h predates the failed experiment.  The experiment was
>> actually error.h, but we've since reworked it as far as practical.  One
>> leftover is still clearly visible: error classes.  Their use is strongly
>> discouraged.
>
>> error.h is for passing around errors within QEMU.  error-report.h is for
>> reporting them to human users via stderr or HMP.  Reporting them via QMP
>> is encapsulated within the QMP monitor code.
>
>> error.h has a few convenience interfaces to report to human users.
>> They're implemented on top of error-report.h.
>
>>> On this basis, I'd simply say that include/qemu/error-report.h is
>>> legacy code that should no longer be used, and that new code should
>>> use include/qapi/error.h exclusively and existing code converted
>>> where practical.
>
>> Absolutely not :)
>
>> 1. Use the simplest suitable method to communicate success / failure
>> within code.  Stick to common methods: non-negative / -1, non-negative /
>> -errno, non-null / null, Error ** parameter.
>
>> Example: when a function returns a non null-pointer on success, and it
>> can fail only 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 Error **.
>
>> Example: when a function's callers need to report details on failure
>> only the function really knows, use Error **, and set suitable errors.
>
>> 2. Use error-report.h to report errors to stderr or an HMP monitor.
>
>> Do not report an error when you're also passing an error for somebody
>> else to handle!  Leave the reporting to the place that consumes the
>> error you pass.
>
> I'm sorry, but I don't see a clear consensus on what should be used.  I ended up
> adding a new special error object named 'error_warn'.  My hope is that this will
> allow most uses of raw "error-report.h" functions (e.g., 'error_printf') and raw
> 'printf' to converge onto "error.h".

I don't see convergence to error.h as a goal.

error.h is for passing errors around, error-report.h is for reporting
them to humans.  Some of error.h's convenience features admittedly blur
the boundary between the two.

> So what about this shorter version for the docs:
>
> --------------------------------------------------
> 7. Error reporting
>
> QEMU provides a variety of interfaces for reporting errors in a uniform
> format. You should use one of these interfaces instead of manually reporting
> them (i.e., do not use 'printf', 'exit' or 'abort').
>
> The simplest form is reporting non-terminating errors (from "qapi/error.h"):
>
>     error_setg(&error_warn, "error for value %d", 1);

&error_warn should be discussed in review of PATCH 1.  Haven't gotten
around to it, sorry.

> There also are two convenience arguments for errors that must terminate QEMU:
>
>     // calls exit()
>     error_setg(&error_fatal, "error for value %d", 1);
>
>     // calls abort() and shows source code information
>     error_setg(&error_abort, "error for value %d", 1);
>
> You should _not_ use terminating functions 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 force
> QEMU to terminate.
>
> The "qapi/error.h" header contains more details for other more complex usage
> patterns (e.g., propagating error messages across functions).
>
> You can also use the location management functions in header
> "qemu/error-report.h" to provide additional information related to inputs
> provided by the user (e.g., command line arguments or configuration files).
> --------------------------------------------------
>
> Thanks,
>   Lluis