Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
Markus Armbruster writes: > "Daniel P. Berrange"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 >>> --- >>> 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(_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". 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(_warn, "error for value %d", 1); There also are two convenience
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
Lluís Vilanovawrites: > Markus Armbruster writes: > >> "Daniel P. Berrange" 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 --- 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(_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: > > -- >
[Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
Gives some general guidelines for reporting errors in QEMU. Signed-off-by: Lluís Vilanova--- 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(_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.
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
Daniel P Berrange writes: [...] > 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. > 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. Mmmm, I've just reviewed both headers and you sound partially right. AFAIU, "qemu/error-report.h" contains the additional logic to manage "input locations", not present anywhere else. Also, you state that only the reporting functions in "qemu/error.h" should be used. Since "qemu/error.h" internally uses 'error_report()' (from "qemu/error-report.h"), it includes the input location information (if any). So, I will simply refer to "qemu/error.h" for the general reporting functions, plus the location management functions in "qemu/error-report.h". Does this sound to follow the expected flow of latest QEMU? Thanks, Lluis -- "And it's much the same thing with knowledge, for whenever you learn something new, the whole world becomes that much richer." -- The Princess of Pure Reason, as told by Norton Juster in The Phantom Tollbooth
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
On Mon, Nov 23, 2015 at 09:05:30PM +0100, Lluís Vilanova wrote: > Daniel P Berrange writes: > [...] > > 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. > > > 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. > > Mmmm, I've just reviewed both headers and you sound partially right. > > AFAIU, "qemu/error-report.h" contains the additional logic to manage "input > locations", not present anywhere else. Also, you state that only the reporting > functions in "qemu/error.h" should be used. > > Since "qemu/error.h" internally uses 'error_report()' (from > "qemu/error-report.h"), it includes the input location information (if any). > So, > I will simply refer to "qemu/error.h" for the general reporting functions, > plus > the location management functions in "qemu/error-report.h". I don't think the location management functions need to be pointed out as broadly speaking, no patches ever need to use them. It should be sufficient to just describe the new error reporting functions in no new code will ever want to use them in general. Really we only need document the qapi/error.h functions and tell people not to use anything else Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
"Daniel P. Berrange"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 >> --- >> 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(_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.
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
"Daniel P. Berrange"writes: > On Mon, Nov 23, 2015 at 09:05:30PM +0100, Lluís Vilanova wrote: >> Daniel P Berrange writes: >> [...] >> > 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. >> >> > 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. >> >> Mmmm, I've just reviewed both headers and you sound partially right. >> >> AFAIU, "qemu/error-report.h" contains the additional logic to manage "input >> locations", not present anywhere else. Also, you state that only the >> reporting >> functions in "qemu/error.h" should be used. >> >> Since "qemu/error.h" internally uses 'error_report()' (from >> "qemu/error-report.h"), it includes the input location information >> (if any). So, >> I will simply refer to "qemu/error.h" for the general reporting >> functions, plus >> the location management functions in "qemu/error-report.h". > > I don't think the location management functions need to be pointed > out as broadly speaking, no patches ever need to use them. It should > be sufficient to just describe the new error reporting functions > in no new code will ever want to use them in general. Really we > only need document the qapi/error.h functions and tell people not > to use anything else People *will* need to use error-report.h to report errors to stderr or HMP monitor in code wherever such reporting is appropriate. Good examples include handling command line options, HMP command handlers. A not so good example would be device emulation code; there we should arguably use a logging interface instead, which we don't have. Counter-examples are a QMP command handler, and pretty much any other code that's expected to pass on its errors for somebody else to handle. Locations are mostly automatic, but there are cases where you can improve error message quality by setting the right location. Underused. A few examples are visible in grep loc_pop().
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
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> --- > 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(_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. 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. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|