Lluís Vilanova <vilan...@ac.upc.edu> writes: > Markus Armbruster writes: > >> "Daniel P. Berrange" <berra...@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 <vilan...@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