| Message ID | 145442966104.1539.5045217656318286174.stgit@localhost |
|---|---|
| State | New |
| Headers | show |
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.
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
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. [...]
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
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.
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 --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.
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(+)