[Qemu-devel] [PATCH] [doc] Introduce coding style for errors

[Lluís Vilanova]

Gives some general guidelines for reporting errors in QEMU.


Signed-off-by: Lluís Vilanova 
---
 HACKING  |   52 ++
 include/qapi/error.h |   12 
 util/error.c |   24 +--
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/HACKING b/HACKING
index 12fbc8a..af36009 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,55 @@ 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(_now, "Error with %s", "arguments");
+
+For convenience, you can also use 'error_setg_errno' and 'error_setg_win32' to
+append a message for OS-specific errors, and 'error_setg_file_open' for errors
+when opening files.
+
+7.3. Errors with an immediate exit/abort
+
+There are two convenience functions to report errors that immediately terminate
+QEMU:
+
+* 'error_setg(_fatal, msg, ...)'
+
+  Reports a fatal error with the given error message and exits QEMU.
+
+* 'error_setg(_abort, msg, ...)'
+
+  Reports a programming error with the given error message and aborts QEMU.
+
+In general, you should report errors but *not* terminate QEMU if the errors are
+(or can be) triggered by guest code (e.g., some unimplemented corner case).
+This also applies to hardware emulation (it is OK to leave the device in a
+non-operational state until next reboot, though). Otherwise that can be abused
+by guest code to terminate QEMU.
+
+In such cases you should use the argument 'error_now'.
diff --git a/include/qapi/error.h b/include/qapi/error.h
index c69dddb..e2bfc19 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -53,6 +53,9 @@
  * Call a function treating errors as fatal:
  * foo(arg, _fatal);
  *
+ * Call a function immediately showing all errors:
+ * foo(arg, _now);
+ *
  * Receive an error and pass it on to the caller:
  * Error *err = NULL;
  * foo(arg, );
@@ -104,6 +107,7 @@ ErrorClass error_get_class(const Error *err);
  * then.
  * If @errp is _abort, print a suitable message and abort().
  * If @errp is _fatal, print a suitable message and exit(1).
+ * If @errp is _now, 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, ...
@@ -154,6 +158,7 @@ void error_setg_win32_internal(Error **errp,
  * abort().
  * Else, if @dst_errp is _fatal, print a suitable message and
  * exit(1).
+ * Else, if @dst_errp is _now, 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.
@@ -217,4 +222,11 @@ extern Error *error_abort;
  */
 extern Error *error_fatal;
 
+/*
+ * Pass to error_setg() & friends to immediately show an error.
+ *
+ * Has the same formatting of #error_abort, but does not terminate QEMU.
+ */
+extern Error *error_now;
+
 #endif
diff --git a/util/error.c b/util/error.c
index 8b86490..e507039 100644
--- a/util/error.c
+++ b/util/error.c
@@ -27,6 +27,7 @@ struct Error
 
 Error *error_abort;
 Error *error_fatal;
+Error *error_now;
 
 static void error_handle_fatal(Error **errp, Error *err)
 {
@@ -62,9 +63,14 @@ static void error_setv(Error **errp,
 err->func = func;
 
 error_handle_fatal(errp, err);
-*errp = err;
-
-errno = saved_errno;
+if (errp == _now) {
+fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
+err->func, err->src, err->line);
+error_report_err(err);
+} else {
+*errp = err;
+errno = saved_errno;
+}
 }
 
 void error_set_internal(Error **errp,
@@ -226,9 +232,15 @@ void error_propagate(Error **dst_errp, Error *local_err)
 return;
 }
 error_handle_fatal(dst_errp, local_err);
-if (dst_errp 

Re: [Qemu-devel] [PATCH] [doc] Introduce coding style for errors

[Eric Blake]

On 11/23/2015 07:18 AM, Lluís Vilanova wrote:
> Gives some general guidelines for reporting errors in QEMU.

This is more than just a doc patch; you are actually proposing new code
in the way of _now.  The commit message must absolutely mention
changes like this, or possibly even split this into two changes
(document existing behavior, then add new code and its matching docs).

By the way, titling your message '[doc]' rather than 'doc:' means that
'git am' will eat the '[doc]' and leave just "Introduce coding style for
errors" as the final subject, which doesn't match usual conventions.

> +++ b/HACKING
> @@ -157,3 +157,55 @@ 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').

There are places where calling exit() is okay (when parsing command-line
arguments), and where abort() is okay (in code that cannot be reached,
such as a dead switch() branch).  So I don't know if this sentence
should be reworded, or if adding even something as simple as "generally"
will weaken it too much.

> +
> +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.
> +

This part describes existing behavior,

> +In its simplest form, you can immediately report an error with:
> +
> +error_setg(_now, "Error with %s", "arguments");

and this is brand new but not mentioned in the commit message.

> +
> +For convenience, you can also use 'error_setg_errno' and 'error_setg_win32' 
> to
> +append a message for OS-specific errors, and 'error_setg_file_open' for 
> errors
> +when opening files.

I'm worried that we may be trying to duplicate too much information, in
which case one will get out of sync.  Merely pointing to error.h may be
good enough for the purposes of HACKING, rather than calling out all the
subtleties (leaving that for error.h).


>  
> +/*
> + * Pass to error_setg() & friends to immediately show an error.
> + *
> + * Has the same formatting of #error_abort, but does not terminate QEMU.

Doesn't that make it useful only for warnings?  If so, should it
automatically prepend "warning: "?  Where do you propose using it, for a
demonstration of why it is useful?

> +++ b/util/error.c
> @@ -27,6 +27,7 @@ struct Error
>  
>  Error *error_abort;
>  Error *error_fatal;
> +Error *error_now;
>  
>  static void error_handle_fatal(Error **errp, Error *err)
>  {
> @@ -62,9 +63,14 @@ static void error_setv(Error **errp,
>  err->func = func;
>  
>  error_handle_fatal(errp, err);
> -*errp = err;
> -
> -errno = saved_errno;
> +if (errp == _now) {
> +fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
> +err->func, err->src, err->line);
> +error_report_err(err);

This lacks timestamping information on the "Unexpected error..." output;
is that going to be a problem?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] [doc] Introduce coding style for errors

[Lluís Vilanova]

Eric Blake writes:

> On 11/23/2015 07:18 AM, Lluís Vilanova wrote:
>> Gives some general guidelines for reporting errors in QEMU.

> This is more than just a doc patch; you are actually proposing new code
> in the way of _now.  The commit message must absolutely mention
> changes like this, or possibly even split this into two changes
> (document existing behavior, then add new code and its matching docs).

Yes, sorry. I added it after writing most of the text, and forgot to update the
patch description. Will resend a fixed version.


[...]
>> +++ b/HACKING
>> @@ -157,3 +157,55 @@ 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').

> There are places where calling exit() is okay (when parsing command-line
> arguments), and where abort() is okay (in code that cannot be reached,
> such as a dead switch() branch).  So I don't know if this sentence
> should be reworded, or if adding even something as simple as "generally"
> will weaken it too much.

Wouldn't it be better to use "error_setg(_fatal, ...)" instead of an exit?
Same applies to 'abort' and 'error_abort'; it additionally provides a message
plus a bit of of information on the failed function/file/line.


[...]
>> +
>> +For convenience, you can also use 'error_setg_errno' and 'error_setg_win32' 
>> to
>> +append a message for OS-specific errors, and 'error_setg_file_open' for 
>> errors
>> +when opening files.

> I'm worried that we may be trying to duplicate too much information, in
> which case one will get out of sync.  Merely pointing to error.h may be
> good enough for the purposes of HACKING, rather than calling out all the
> subtleties (leaving that for error.h).

Aha. I can simply reference the three special error objects, and leave the rest
to error.h.


>> 
>> +/*
>> + * Pass to error_setg() & friends to immediately show an error.
>> + *
>> + * Has the same formatting of #error_abort, but does not terminate QEMU.

> Doesn't that make it useful only for warnings?  If so, should it
> automatically prepend "warning: "?  Where do you propose using it, for a
> demonstration of why it is useful?

Actually, this new object caters to some comments I received, where guest and
device code should never terminate QEMU. To me it sounds like a non-terminating
abort, but I can certainly rename to object to 'errror_warn' and prepend the
appropriate "warning: " text.


>> +++ b/util/error.c
>> @@ -27,6 +27,7 @@ struct Error
>> 
>> Error *error_abort;
>> Error *error_fatal;
>> +Error *error_now;
>> 
>> static void error_handle_fatal(Error **errp, Error *err)
>> {
>> @@ -62,9 +63,14 @@ static void error_setv(Error **errp,
err-> func = func;
>> 
>> error_handle_fatal(errp, err);
>> -*errp = err;
>> -
>> -errno = saved_errno;
>> +if (errp == _now) {
>> +fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
>> +err->func, err->src, err->line);
>> +error_report_err(err);

> This lacks timestamping information on the "Unexpected error..." output;
> is that going to be a problem?

The code mimics 'error_handle_fatal', which does not show any timestamp.


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