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 &error_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(&error_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 == &error_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