utils: Improve and document error reporting
RFC PATCH v3 1/1 utils: Improve and document error reporting
Adds support for reporting non-terminating errors (dubbed warnings), and briefly documents what routines should developers generally use to keep error reporting consistent.
Changes in v3
=============
* Drop special object 'error_warn' in favour of raw 'error_report()' [suggested by Markus Armbruster].
Changes in v2
=============
* Split in two patches.
* Explicitly add a warning error object.
---
Lluís Vilanova (1):
doc: Introduce coding style for errors
HACKING | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
To: [email protected]
Cc: Stefan Hajnoczi <[email protected]>
Cc: Dr. David Alan Gilbert <[email protected]>
Cc: Thomas Huth <[email protected]>
Cc: Markus Armbruster <[email protected]>
Cc: Eric Blake <[email protected]>
PATCH v3 1/1 doc: Introduce coding style for errors
Gives some general guidelines for reporting errors in QEMU.
--- HACKING | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
HACKING +36 -0
@@ -157,3 +157,39 @@ 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_fatal, "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. + +WARNING: Do *not* use 'error_fatal' or 'error_abort' for errors that are (or can +be) triggered by guest code (e.g., some unimplimented corner case in guest code +translation or device code). Otherwise that can be abused by guest code to +terminate QEMU. Instead, you should use the 'error_report()' routine.
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 9 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>--- HACKING | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)+7.1. Errors in user inputs
HACKING painful. These are: · line 174
Quoted 7 lines
HACKING +7
-0+ +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.
s/on/in/
HACKING painful. These are: · line 179
Quoted 5 lines
HACKING +5
-0+ +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).
Maybe: "not directly triggered". After all, we DO have places where Error is used which can ultimately be traced to a user command (such as in QMP commands), but where the local code can also be called internally; the use of Error at the local level then lets us leave it up to the caller whether to report a message (because the caller has more context).
HACKING painful. These are: · line 186
Quoted 7 lines
HACKING +7
-0+ +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_fatal, "Error with %s", "arguments");
This paradigm doesn't appear anywhere in the current code base (hw/ppc/spapr*.c has a few cases of error_setg(&error_abort), but nothing directly passes error_fatal). It's a bit odd to document something that isn't actually used.
HACKING painful. These are: · line 189
Quoted 3 lines
HACKING +3
-0+ +See the "include/qapi/error.h" header for additional convenience functions and +special arguments. Specially, see 'error_fatal' and 'error_abort' to show errors
s/Specially/Specifically/
HACKING painful. These are: · line 193
Quoted 4 lines
HACKING +4
-0+and immediately terminate QEMU. + +WARNING: Do *not* use 'error_fatal' or 'error_abort' for errors that are (or can +be) triggered by guest code (e.g., some unimplimented corner case in guest code
s/unimplimented/unimplemented/
HACKING painful. These are: · line 195
Quoted 2 lines
HACKING +2
-0+translation or device code). Otherwise that can be abused by guest code to +terminate QEMU. Instead, you should use the 'error_report()' routine.
But a definite improvement over v2. I think we're getting closer to a good summary.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>--- HACKING | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
...
Quoted 11 lines
HACKING painful. These are: · line 186
Quoted 6 lines
HACKING +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper error_report_fatal() function instead.
Thomas
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 21 lines · Thomas Huth
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 36 ++++++++++++++++++++++++++++++++++++1 file changed, 36 insertions(+)...
Quoted 11 lines
HACKING painful. These are: · line 186
Quoted 6 lines
HACKING +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.
Quoted 3 lines
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper
error_report_fatal()function instead.
That's a bit of a chicken and egg problem. My main intention was to provide a best practices summary on reporting messages/errors, since QEMU's code is really heterogeneous on that regard. But there seems to be no consensus on some details of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would definitiely be an improvement IMHO, but I dont have time to adapt existing code to these (and I was told not to add unused utility functions for this).
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist. message_report(fmt, ...)
// Generate message with prepended file/line information for the caller. // Calls exit/abort on the last two. error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)
But, should I add these without providing extensive patches that refactor code to use them?
Cheers,
Lluis
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 32 lines · Lluís Vilanova
Quoted 21 lines · Thomas Huth
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 36 ++++++++++++++++++++++++++++++++++++1 file changed, 36 insertions(+)...
Quoted 11 lines
HACKING painful. These are: · line 186
Quoted 6 lines
HACKING +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.Quoted 3 lines
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper
error_report_fatal()function instead.That's a bit of a chicken and egg problem. My main intention was to provide a best practices summary on reporting messages/errors, since QEMU's code is really heterogeneous on that regard. But there seems to be no consensus on some details of what the proper way should be with the current interfaces.
Quoted 8 lines
Utility functions for "regular messages", warnings, fatals and aborts would definitiely be an improvement IMHO, but I dont have time to adapt existing code to these (and I was told not to add unused utility functions for this).
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)
Not sure what this should be good for? We've already got error_report() that generates messages "as is", haven't we?
Quoted 9 lines
// Generate message with prepended file/line information for the caller. // Calls exit/abort on the last two. error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)But, should I add these without providing extensive patches that refactor code to use them?
Maybe create a patch that introduces the _fatal and _abort functions (I'd skip the _warn functions for now), and use them in one or two files (e.g. replace the error_setg(&error_abort, ...) in spapr.c). That should not be that much of work, and could be a good base for further discussion?
Thomas
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 37 lines · Thomas Huth
Quoted 36 lines · Lluís Vilanova
Quoted 21 lines · Thomas Huth
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 36 ++++++++++++++++++++++++++++++++++++1 file changed, 36 insertions(+)...
Quoted 11 lines
HACKING painful. These are: · line 186
Quoted 6 lines
HACKING +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.Quoted 3 lines
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper
error_report_fatal()function instead.That's a bit of a chicken and egg problem. My main intention was to provide a best practices summary on reporting messages/errors, since QEMU's code is really heterogeneous on that regard. But there seems to be no consensus on some details of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would definitiely be an improvement IMHO, but I dont have time to adapt existing code to these (and I was told not to add unused utility functions for this).
Quoted 7 lines
Quoted 4 lines
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)Not sure what this should be good for? We've already got
error_report()that generates messages "as is", haven't we?
Well, it just seemed wrong to me using error_report() to report "regular messages" :)
Quoted 7 lines
Quoted 7 lines
// Generate message with prepended file/line information for the caller. // Calls exit/abort on the last two. error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)
Quoted 7 lines
Quoted 2 lines
But, should I add these without providing extensive patches that refactor code to use them?
Maybe create a patch that introduces the
_fataland_abortfunctions (I'd skip the_warnfunctions for now), and use them in one or two files (e.g. replace theerror_setg(&error_abort, ...)inspapr.c). That should not be that much of work, and could be a good base for further discussion?
I can do that. But then should 'error_fatal' and 'error_abort' be officially deprecated in favour of error_report_fatal() and error_report_abort()?
Cheers,
Lluis
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 60 lines · Lluís Vilanova
Quoted 37 lines · Thomas Huth
Quoted 36 lines · Lluís Vilanova
Quoted 21 lines · Thomas Huth
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 36 ++++++++++++++++++++++++++++++++++++1 file changed, 36 insertions(+)...
Quoted 11 lines
HACKING painful. These are: · line 186
Quoted 6 lines
HACKING +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.Quoted 3 lines
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper
error_report_fatal()function instead.That's a bit of a chicken and egg problem. My main intention was to provide a best practices summary on reporting messages/errors, since QEMU's code is really heterogeneous on that regard. But there seems to be no consensus on some details of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would definitiely be an improvement IMHO, but I dont have time to adapt existing code to these (and I was told not to add unused utility functions for this).
Quoted 7 lines
Quoted 4 lines
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)Not sure what this should be good for? We've already got
error_report()that generates messages "as is", haven't we?Well, it just seemed wrong to me using
error_report()to report "regular messages" :)Quoted 9 lines
Quoted 9 lines
// Generate message with prepended file/line information for the caller. // Calls exit/abort on the last two. error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)But, should I add these without providing extensive patches that refactor code to use them?
Quoted 7 lines
Quoted 4 lines
Maybe create a patch that introduces the
_fataland_abortfunctions (I'd skip the_warnfunctions for now), and use them in one or two files (e.g. replace theerror_setg(&error_abort, ...)inspapr.c). That should not be that much of work, and could be a good base for further discussion?I can do that. But then should '
error_fatal' and 'error_abort' be officially deprecated in favour oferror_report_fatal()anderror_report_abort()?
Sorry, I see this is misleading. I mean deprecate directly using "error_setg(error_fatal)"; you can still decide to pass error_fatal as an error object to other user functions.
Cheers,
Lluis
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 67 lines · Lluís Vilanova
Quoted 64 lines · Lluís Vilanova
Quoted 37 lines · Thomas Huth
Quoted 36 lines · Lluís Vilanova
Quoted 21 lines · Thomas Huth
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 36 ++++++++++++++++++++++++++++++++++++1 file changed, 36 insertions(+)...
Quoted 11 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.Quoted 3 lines
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper
error_report_fatal()function instead.That's a bit of a chicken and egg problem. My main intention was to provide a best practices summary on reporting messages/errors, since QEMU's code is really heterogeneous on that regard. But there seems to be no consensus on some details of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would definitiely be an improvement IMHO, but I dont have time to adapt existing code to these (and I was told not to add unused utility functions for this).
Quoted 7 lines
Quoted 4 lines
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)Not sure what this should be good for? We've already got
error_report()that generates messages "as is", haven't we?Well, it just seemed wrong to me using
error_report()to report "regular messages" :)Quoted 14 lines
Quoted 9 lines
// Generate message with prepended file/line information for the caller. // Calls exit/abort on the last two. error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)But, should I add these without providing extensive patches that refactor code to use them?
Maybe create a patch that introduces the
_fataland_abortfunctions (I'd skip the_warnfunctions for now), and use them in one or two files (e.g. replace theerror_setg(&error_abort, ...)inspapr.c). That should not be that much of work, and could be a good base for further discussion?
Quoted 6 lines
Quoted 2 lines
I can do that. But then should '
error_fatal' and 'error_abort' be officially deprecated in favour oferror_report_fatal()anderror_report_abort()?Sorry, I see this is misleading. I mean deprecate directly using "
error_setg(error_fatal)"; you can still decide to passerror_fatalas an error object to other user functions.
Since we hardly got any of these in the code right now, I don't see an urgent need to explicitely say that this should be deprecated. I hope that people rather will use the new functions automatically instead since these sounds much more intuitive, IMHO.
Thomas
Re: [Qemu-devel] [PATCH v3 ] doc: Introduce coding style for errors
Quoted 71 lines · Thomas Huth
Quoted 70 lines · Lluís Vilanova
Quoted 67 lines · Lluís Vilanova
Quoted 37 lines · Thomas Huth
Quoted 36 lines · Lluís Vilanova
Quoted 21 lines · Thomas Huth
Quoted 7 lines · Eric Blake
Quoted 6 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 36 ++++++++++++++++++++++++++++++++++++1 file changed, 36 insertions(+)...
Quoted 11 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+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_fatal, "Error with %s", "arguments");This paradigm doesn't appear anywhere in the current code base (
hw/ppc/spapr*.chas a few cases oferror_setg(&error_abort), but nothing directly passeserror_fatal). It's a bit odd to document something that isn't actually used.Quoted 3 lines
+1 for _not_ documenting this here: IMHO this looks ugly. If we want something like this, I think we should introduce a proper
error_report_fatal()function instead.That's a bit of a chicken and egg problem. My main intention was to provide a best practices summary on reporting messages/errors, since QEMU's code is really heterogeneous on that regard. But there seems to be no consensus on some details of what the proper way should be with the current interfaces.
Utility functions for "regular messages", warnings, fatals and aborts would definitiely be an improvement IMHO, but I dont have time to adapt existing code to these (and I was told not to add unused utility functions for this).
Quoted 7 lines
Quoted 4 lines
Now, if I were able to add such functions, it'd be something like:
// Generate message "as is"; not sure if this should exist.
message_report(fmt, ...)Not sure what this should be good for? We've already got
error_report()that generates messages "as is", haven't we?Well, it just seemed wrong to me using
error_report()to report "regular messages" :)Quoted 7 lines
Quoted 7 lines
// Generate message with prepended file/line information for the caller. // Calls exit/abort on the last two. error_report_{warn,fatal,abort}(fmt, ...)
// Same with an added message from strerror.
error_report_{warn,fatal,abort}_errno(fmt, ...)Quoted 7 lines
Quoted 2 lines
But, should I add these without providing extensive patches that refactor code to use them?
Maybe create a patch that introduces the
_fataland_abortfunctions (I'd skip the_warnfunctions for now), and use them in one or two files (e.g. replace theerror_setg(&error_abort, ...)inspapr.c). That should not be that much of work, and could be a good base for further discussion?I can do that. But then should '
error_fatal' and 'error_abort' be officially deprecated in favour oferror_report_fatal()anderror_report_abort()?
Quoted 8 lines
Quoted 3 lines
Sorry, I see this is misleading. I mean deprecate directly using "
error_setg(error_fatal)"; you can still decide to passerror_fatalas an error object to other user functions.Since we hardly got any of these in the code right now, I don't see an urgent need to explicitely say that this should be deprecated. I hope that people rather will use the new functions automatically instead since these sounds much more intuitive, IMHO.
Got it. I'm writing some of them now.
Cheers,
Lluis