utils: Improve and document error reporting
v2 cover 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 v2
=============
* Split in two patches.
* Explicitly add a warning error object.
---
Lluís Vilanova (2):
utils: Add warning messages
doc: Introduce coding style for errors
HACKING | 31 +++++++++++++++++++++++++++++++ include/qapi/error.h | 20 ++++++++++++++++++++ util/error.c | 37 +++++++++++++++++++++++++++---------- 3 files changed, 78 insertions(+), 10 deletions(-)
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 v2 1/2 utils: Add warning messages
Adds a special error object that transforms error messages into immediately reported warnings.
--- include/qapi/error.h | 20 ++++++++++++++++++++ util/error.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 10 deletions(-)
include/qapi/error.h +20 -0
@@ -57,6 +57,9 @@* Call a function treating errors as fatal: * foo(arg, &error_fatal); * +* Call a function immediately showing all errors as warnings: +* foo(arg, &error_warn); +* * Receive an error and pass it on to the caller: * Error *err = NULL; * foo(arg, &err);
@@ -108,6 +111,7 @@ ErrorClass error_get_class(const Error *err);* then. * If @errp is &error_abort, print a suitable message and abort(). * If @errp is &error_fatal, print a suitable message and exit(1). +* If @errp is &error_warn, print a suitable message. * If @errp is anything else, *@errp must be NULL. * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its * human-readable error message is made from printf-style @fmt, ...
@@ -158,6 +162,7 @@ void error_setg_win32_internal(Error **errp,* abort(). * Else, if @dst_errp is &error_fatal, print a suitable message and * exit(1). +* Else, if @dst_errp is &error_warn, print a suitable message. * Else, if @dst_errp already contains an error, ignore this one: free * the error object. * Else, move the error object from @local_err to *@dst_errp.
@@ -218,12 +223,27 @@ void error_set_internal(Error **errp,/* * Pass to error_setg() & friends to abort() on error. +* +* WARNING: Do _not_ use 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. */ extern Error *error_abort;/* * Pass to error_setg() & friends to exit(1) on error. +* +* WARNING: Do _not_ use 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. */ extern Error *error_fatal; +/* +* Pass to error_setg() & friends to immediately show an error as a warning. +*/ +extern Error *error_warn; +#endif
util/error.c +27 -10
@@ -15,6 +15,7 @@#include "qemu-common.h" #include "qapi/error.h" #include "qemu/error-report.h" +#include "qemu/timer.h" struct Error {
@@ -27,8 +28,9 @@ struct ErrorError *error_abort;Error *error_fatal; +Error *error_warn; -static void error_handle_fatal (Error **errp,Error *err) +static bool error_handle_fatal (Error **errp,Error *err) {if (errp == &error_abort) { fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
@@ -40,6 +42,20 @@ static void error_handle_fatal(Error **errp, Error *err)error_report_err (err);exit (1 ); } + +if (errp == &error_warn) { +/* cannot use error_report_err() because it adds newlines */ +error_report ("warning: [%s() at %s:%d] %s" , + err->func, err->src, err->line, err->msg); +if (err->hint) { +error_printf_unless_qmp ("warning: [%s() at %s:%d] %s" , + err->func, err->src, err->line, + err->hint->str); + } +return true; + }else { +return false; + } } static void error_setv(Error **errp,
@@ -61,10 +77,10 @@ static void error_setv(Error **errp, err->line = line; err->func = func; -error_handle_fatal (errp, err); - *errp = err; - - errno = saved_errno; +if (!error_handle_fatal (errp, err)) { + *errp = err; + errno = saved_errno; + } } void error_set_internal(Error **errp,
@@ -232,10 +248,11 @@ void error_propagate(Error **dst_errp, Error *local_err)if (!local_err) {return ; } -error_handle_fatal (dst_errp, local_err); -if (dst_errp && !*dst_errp) { - *dst_errp = local_err; - }else { -error_free (local_err); +if (!error_handle_fatal (dst_errp, local_err)) { +if (dst_errp && !*dst_errp) { + *dst_errp = local_err; + }else { +error_free (local_err); + } } }
Re: [Qemu-devel] [PATCH v2 1/2] utils: Add warning messages
Quoted 2 lines · Lluís Vilanova
Adds a special error object that transforms error messages into immediately reported warnings.
Before I dive into details: my fundamental objection is that &error_warn is new infrastructure without a use. The new "this is how you should do warnings" paragraph in error.h's big comment does not count as use :)
Without actual use, we can't be sure it's useful. And being useful should be mandatory for new infrastructure, even when it's as small as this one.
Finally, note that
err = NULL;
foo(arg, &err);
if (err) {
error_report("warning: %s", error_get_pretty(err));
error_free(err);
}and
foo(arg, &error_warn)
are subtly different.
One, the former throws away the hint. That may or may not be appropriate. It also happens in other places that amend an error's message. Perhaps we could use a function to amend an error's message. To find out, we'd have to track down the places that do that now.
Two, the latter doesn't let the caller see whether foo() warned. I believe this makes it unsuitable for some cases, or in other words, it's insufficiently general.
Three, the latter can't catch misuse the former catches, namely multiple error_set(). The latter happily reports all of them, the former fails error_setv()'s assertion for the second one.
If warnings are common enough to justify infrastructure, then I figure a convenience function to report an Error as a warning similar to error_report_err() would be more general and less prone to misuse.
Quoted 47 lines
Signed-off-by Lluís Vilanova <[email protected]>--- include/qapi/error.h | 20 ++++++++++++++++++++ util/error.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 10 deletions(-)include/qapi/error.h +10
-0@@ -57,6 +57,9 @@* Call a function treating errors as fatal: * foo(arg, &error_fatal); * +* Call a function immediately showing all errors as warnings: +* foo(arg, &error_warn); +* * Receive an error and pass it on to the caller: * Error *err = NULL; * foo(arg, &err); @@ -108,6 +111,7 @@ ErrorClass error_get_class(const Error *err);* then. * If @errp is &error_abort, print a suitable message and abort(). * If @errp is &error_fatal, print a suitable message and exit(1). +* If @errp is &error_warn, print a suitable message. * If @errp is anything else, *@errp must be NULL. * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its * human-readable error message is made from printf-style @fmt, ... @@ -158,6 +162,7 @@ void error_setg_win32_internal(Error **errp,* abort(). * Else, if @dst_errp is &error_fatal, print a suitable message and * exit(1). +* Else, if @dst_errp is &error_warn, print a suitable message. * Else, if @dst_errp already contains an error, ignore this one: free * the error object. * Else, move the error object from @local_err to *@dst_errp. @@ -218,12 +223,27 @@ void error_set_internal(Error **errp,/* * Pass to error_setg() & friends to abort() on error. +* +* WARNING: Do _not_ use 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. */ extern Error *error_abort;
include/qapi/error.h void error_set_internal(Error **errp, · line 242
Quoted 10 lines
include/qapi/error.h +5
-0/* * Pass to error_setg() & friends to exit(1) on error. +* +* WARNING: Do _not_ use 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. */ extern Error *error_fatal;
This hunk isn't covered by the commit message.
Similar admonitions elsewhere in this file are less ornately decorated. Plain
* Do *not* use 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.
would do, and avoid the long lines.
Except this wording is too narrow. There are many more places where "terminate on error" is wrong, such as monitor commands.
&error_exit is okay exactly when and where exit(1) is okay: on configuration error during startup, and on certain unrecoverable errors where it's unsafe to continue.
Likewise, &error_abort is okay exactly when and where abort() is okay:
mostly on programming errors. Possibly also on "unexpected" unrecoverable errors where it's unsafe to continue; we did that for most memory allocation failures at some time, not sure we still do.
Perhaps we need to advise on proper use of abort() and exit(1), but I doubt error.h is the right place to do it.
Perhaps we need to remind users some more that &error_exit is just like exit(1) and &error_abort is just like abort(). Doubtful.
If we want to, I'd recommend a separate patch.
Quoted 31 lines
include/qapi/error.h +5
-0+/* +* Pass to error_setg() & friends to immediately show an error as a warning. +*/ +extern Error *error_warn; +#endif util/error.c +3
-1@@ -15,6 +15,7 @@#include "qemu-common.h" #include "qapi/error.h" #include "qemu/error-report.h" +#include "qemu/timer.h" struct Error {@@ -27,8 +28,9 @@ struct ErrorError *error_abort;Error *error_fatal; +Error *error_warn; -static void error_handle_fatal (Error **errp,Error *err) +static bool error_handle_fatal (Error **errp,Error *err) {if (errp == &error_abort) { fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
util/error.c static void error_handle_fatal(Error **errp, Error *err) · line 49
Quoted 8 lines
util/error.c +5
-0@@ -40,6 +42,20 @@ static void error_handle_fatal(Error **errp, Error *err)error_report_err (err);exit (1 ); } + +if (errp == &error_warn) { +/* cannot use error_report_err() because it adds newlines */ +error_report ("warning: [%s() at %s:%d] %s" , + err->func, err->src, err->line, err->msg);
I don't think we should show err->func & friends.
We do for &error_abort, because that's a message for developers.
We don't for &error_exit, because that's for users.
util/error.c static void error_handle_fatal(Error **errp, Error *err) · line 59
Quoted 10 lines
util/error.c +9
-0+if (err->hint) { +error_printf_unless_qmp ("warning: [%s() at %s:%d] %s" , + err->func, err->src, err->line, + err->hint->str); + } +return true; + }else { +return false; + } }
The function's name is now misleading.
Quoted 5 lines
util/error.c +0
-0static void error_setv(Error **errp,@@ -61,10 +77,10 @@ static void error_setv(Error **errp, err->line = line; err->func = func;
util/error.c static void error_setv(Error **errp, · line 84
Quoted 10 lines
util/error.c +4
-4-error_handle_fatal (errp, err); - *errp = err; - - errno = saved_errno; +if (!error_handle_fatal (errp, err)) { + *errp = err; + errno = saved_errno; + } }
Awkward.
Quoted 9 lines
util/error.c +0
-3void error_set_internal(Error **errp,@@ -232,10 +248,11 @@ void error_propagate(Error **dst_errp, Error *local_err)if (!local_err) {return ; } -error_handle_fatal (dst_errp, local_err); -if (dst_errp && !*dst_errp) { - *dst_errp = local_err;
util/error.c void error_propagate(Error **dst_errp, Error *local_err) · line 258
Quoted 10 lines
util/error.c +6
-2- }else { -error_free (local_err); +if (!error_handle_fatal (dst_errp, local_err)) { +if (dst_errp && !*dst_errp) { + *dst_errp = local_err; + }else { +error_free (local_err); + } } }
Just as awkward.
Re: [Qemu-devel] [PATCH v2 1/2] utils: Add warning messages
Quoted 36 lines · Markus Armbruster
Quoted 2 lines · Lluís Vilanova
Adds a special error object that transforms error messages into immediately reported warnings.
Before I dive into details: my fundamental objection is that
&error_warnis new infrastructure without a use. The new "this is how you should do warnings" paragraph in error.h's big comment does not count as use :)Without actual use, we can't be sure it's useful. And being useful should be mandatory for new infrastructure, even when it's as small as this one.
Finally, note that
err = NULL; foo(arg, &err); if (err) { error_report("warning: %s", error_get_pretty(err)); error_free(err); }and
foo(arg, &error_warn)are subtly different.
One, the former throws away the hint. That may or may not be appropriate. It also happens in other places that amend an error's message. Perhaps we could use a function to amend an error's message. To find out, we'd have to track down the places that do that now.
Two, the latter doesn't let the caller see whether
foo()warned. I believe this makes it unsuitable for some cases, or in other words, it's insufficiently general.
Quoted 7 lines
Three, the latter can't catch misuse the former catches, namely multiple
error_set(). The latter happily reports all of them, the former fails error_setv()'s assertion for the second one.If warnings are common enough to justify infrastructure, then I figure a convenience function to report an Error as a warning similar to
error_report_err()would be more general and less prone to misuse.
Certainly true. The target was to avoid raw uses of the more verbose error_* routines in the most common cases, concentrating most uses onto error_setg. If I eliminate the new 'error_warn' then there is no clear and consistent formatting substitute for printf.
Since I do not have time to port existing printf's into 'error_warn', I can just drop this patch. Instead, we can simply admonish developers to use 'error_report' instead of plain 'printf' (although formatting will vary).
Also, using anything that is non-trivially more complex than a printf will probably shy away developers from using it.
Quoted 82 lines
Quoted 47 lines
Signed-off-by Lluís Vilanova <[email protected]>---
include/qapi/error.h | 20 ++++++++++++++++++++ util/error.c | 37 +++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 10 deletions(-)include/qapi/error.h +10
-0@@ -57,6 +57,9 @@* Call a function treating errors as fatal: * foo(arg, &error_fatal); * +* Call a function immediately showing all errors as warnings: +* foo(arg, &error_warn); +* * Receive an error and pass it on to the caller: * Error *err = NULL; * foo(arg, &err); @@ -108,6 +111,7 @@ ErrorClass error_get_class(const Error *err);* then. * If @errp is &error_abort, print a suitable message and abort(). * If @errp is &error_fatal, print a suitable message and exit(1). +* If @errp is &error_warn, print a suitable message. * If @errp is anything else, *@errp must be NULL. * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its * human-readable error message is made from printf-style @fmt, ... @@ -158,6 +162,7 @@ void error_setg_win32_internal(Error **errp,* abort(). * Else, if @dst_errp is &error_fatal, print a suitable message and * exit(1). +* Else, if @dst_errp is &error_warn, print a suitable message. * Else, if @dst_errp already contains an error, ignore this one: free * the error object. * Else, move the error object from @local_err to *@dst_errp. @@ -218,12 +223,27 @@ void error_set_internal(Error **errp,/* * Pass to error_setg() & friends to abort() on error. +* +* WARNING: Do _not_ use 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. */ extern Error *error_abort;include/qapi/error.h void error_set_internal(Error **errp, · line 242
Quoted 10 lines
include/qapi/error.h +5
-0/* * Pass to error_setg() & friends to exit(1) on error. +* +* WARNING: Do _not_ use 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. */ extern Error *error_fatal;This hunk isn't covered by the commit message.
Similar admonitions elsewhere in this file are less ornately decorated. Plain
* Do *not* use 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.would do, and avoid the long lines.
Except this wording is too narrow. There are many more places where "terminate on error" is wrong, such as monitor commands.
&error_exitis okay exactly when and where exit(1) is okay: on configuration error during startup, and on certain unrecoverable errors where it's unsafe to continue.Likewise,
&error_abortis okay exactly when and whereabort()is okay:
mostly on programming errors. Possibly also on "unexpected" unrecoverable errors where it's unsafe to continue; we did that for most memory allocation failures at some time, not sure we still do.
Quoted 7 lines
Perhaps we need to advise on proper use of
abort()and exit(1), but I doubterror.his the right place to do it.Perhaps we need to remind users some more that
&error_exitis just like exit(1) and&error_abortis just likeabort(). Doubtful.If we want to, I'd recommend a separate patch.
[...]
Hmm, then I'll remove those advices and merge your comments into HACKING (which tries to provide very high-level guidelines of when it's safe to exit/abort). In fact the advices on the comments are mere reminders of the text there.
Thanks,
Lluis
PATCH v2 2/2 doc: Introduce coding style for errors
In reply to Lluís VilanovaGives some general guidelines for reporting errors in QEMU.
--- HACKING | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
HACKING +31 -0
@@ -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.
Re: [Qemu-devel] [PATCH v2 2/2] doc: Introduce coding style for errors
Quoted 36 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>--- HACKING | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)HACKING +21
-0@@ -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). +
HACKING painful. These are: · line 190
Quoted 10 lines
HACKING +10
-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_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
Signature · 4 lines
|: 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:
[...]
Quoted 5 lines
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.
Quoted 8 lines
AFAIK,
include/qemu/error-report.his the historical failed experiment in structured error reporting, whileinclude/qapi/error.his the new preferred error reporting system that everything should be using.On this basis, I'd simply say that
include/qemu/error-report.his legacy code that should no longer be used, and that new code should useinclude/qapi/error.hexclusively 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
Signature · 4 lines
"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
Quoted 18 lines · Lluís Vilanova
Daniel P Berrange writes:
[...]Quoted 5 lines
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.
Quoted 8 lines
AFAIK,
include/qemu/error-report.his the historical failed experiment in structured error reporting, whileinclude/qapi/error.his the new preferred error reporting system that everything should be using.On this basis, I'd simply say that
include/qemu/error-report.his legacy code that should no longer be used, and that new code should useinclude/qapi/error.hexclusively and existing code converted where practical.Mmmm, I've just reviewed both headers and you sound partially right.
Quoted 8 lines
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
Signature · 4 lines
|: 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
Quoted 30 lines · Daniel P. Berrange
Quoted 22 lines · Lluís Vilanova
Daniel P Berrange writes:
[...]Quoted 5 lines
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.
Quoted 8 lines
AFAIK,
include/qemu/error-report.his the historical failed experiment in structured error reporting, whileinclude/qapi/error.his the new preferred error reporting system that everything should be using.On this basis, I'd simply say that
include/qemu/error-report.his legacy code that should no longer be used, and that new code should useinclude/qapi/error.hexclusively 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.Quoted 6 lines
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".
Quoted 6 lines
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.hfunctions 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
In reply to Daniel P. BerrangeQuoted 53 lines · Daniel P. Berrange
Quoted 36 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>--- HACKING | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)HACKING +21
-0@@ -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). +HACKING painful. These are: · line 190
Quoted 10 lines
HACKING +10
-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_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.
Quoted 3 lines
AFAIK,
include/qemu/error-report.his the historical failed experiment in structured error reporting, whileinclude/qapi/error.his 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.
Quoted 4 lines
On this basis, I'd simply say that
include/qemu/error-report.his legacy code that should no longer be used, and that new code should useinclude/qapi/error.hexclusively 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
Quoted 88 lines · Markus Armbruster
Quoted 53 lines · Daniel P. Berrange
Quoted 36 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 31 +++++++++++++++++++++++++++++++1 file changed, 31 insertions(+)HACKING +21
-0@@ -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). +HACKING painful. These are: · line 190
Quoted 10 lines
HACKING +10
-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_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.
Quoted 3 lines
AFAIK,
include/qemu/error-report.his the historical failed experiment in structured error reporting, whileinclude/qapi/error.his the new preferred error reporting system that everything should be using.Not quite :)
error-report.hpredates the failed experiment. The experiment was actuallyerror.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.his for passing around errors within QEMU.error-report.his for reporting them to human users via stderr or HMP. Reporting them via QMP is encapsulated within the QMP monitor code.
error.hhas a few convenience interfaces to report to human users. They're implemented on top oferror-report.h.Quoted 4 lines
On this basis, I'd simply say that
include/qemu/error-report.his legacy code that should no longer be used, and that new code should useinclude/qapi/error.hexclusively 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 **.
Quoted 8 lines
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.hto 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(&error_warn, "error for value %d", 1);
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
Signature · 4 lines
"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
Quoted 104 lines · Lluís Vilanova
Quoted 101 lines · Markus Armbruster
Quoted 62 lines · Daniel P. Berrange
Quoted 51 lines · Lluís Vilanova
Gives some general guidelines for reporting errors in QEMU.
Signed-off-by Lluís Vilanova <[email protected]>---
HACKING | 31 +++++++++++++++++++++++++++++++1 file changed, 31 insertions(+)HACKING +4
-0@@ -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. Youshould use one
HACKING +6
-0+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" toreport errors
HACKING +4
-0+related to inputs provided by the user (e.g., command line arguments or +configuration files). + +These functions generate error messages with a uniform formatthat can reference
HACKING +5
-0+a location on the offending input. + +7.2. Other errors + +QEMU provides the functions in "include/qapi/error.h" to reportother types of
+errors (i.e., not triggered by command line arguments or configuration files).HACKING +2
-0+ +Functions in this header are used to accumulate error messages inan 'Error'
+object, which can be propagated up the call chain where it is finally reported.HACKING +6
-0+ +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 convenienceQuoted 4 lines
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.
Quoted 3 lines
AFAIK,
include/qemu/error-report.his the historical failed experiment in structured error reporting, whileinclude/qapi/error.his the new preferred error reporting system that everything should be using.Not quite :)
error-report.hpredates the failed experiment. The experiment was actuallyerror.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.his for passing around errors within QEMU.error-report.his for reporting them to human users via stderr or HMP. Reporting them via QMP is encapsulated within the QMP monitor code.
error.hhas a few convenience interfaces to report to human users. They're implemented on top oferror-report.h.Quoted 4 lines
On this basis, I'd simply say that
include/qemu/error-report.his legacy code that should no longer be used, and that new code should useinclude/qapi/error.hexclusively 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.hto report errors to stderr or an HMP monitor.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 5 lines
So what about this shorter version for the docs:
--------------------------------------------------
7. Error reporting
Quoted 7 lines
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.
Quoted 23 lines
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