Re: [Qemu-devel] [RFC PATCH v1 04/25] error: Add support for multiple errors

[Eric Blake]

On 09/10/2015 11:33 PM, Peter Crosthwaite wrote:
> Allow errors to stack. If an error is already set, don't assert,
> instead, form a linked list. Recent errors are at the front of the
> list, older ones at the back.
> 
> The assertion against the destination erro already being set is

s/erro/error/

> removed.

Do we want to do that blindly, or do we want a design where users must
explicitly ask for nested errors?

I'm wondering aloud here: (haven't actually thought too hard, but typing
as I go)

Since your goal was reducing boilerplate, is there some way we could have:

myfunc1(..., error_add(errp));
myfunc2(..., error_add(errp));

be some way to mark errp as allowing error concatenation? That is,
error_add() would return errp; if *errp was NULL it does nothing
further, but *errp is non-NULL it then sets a flag in errp that it is
okay for further errors to be concatenated.  Then when setting or
propagating an error, we can use the flag within errp to determine if
the caller is okay with us appending to the existing error, or whether
there may be a programming error in that we are detecting a followup
error to an errp that wasn't properly cleared earlier.

Or maybe:

error_start_chaining(errp);
myfunc1(..., errp);
myfunc2(..., errp);
error_stop_chaining(errp);

where we use a counter of how many requests (since myfunc1() may itself
call nested start/stop, so chaining is okay if the counter is non-zero).

> 
> copy/free are all to call their functionality recursively.
> 
> Propagation is implemented as concatenation of two error lists.
> 
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
> 
>  qapi/common.json |  5 ++++-
>  util/error.c     | 27 +++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/qapi/common.json b/qapi/common.json
> index bad56bf..04175db 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -22,11 +22,14 @@
>  # @KVMMissingCap: the requested operation can't be fulfilled because a
>  #                 required KVM capability is missing
>  #
> +# @MultipleErrors: Multiple errors have occured

s/occured/occurred/

Missing a (since 2.5) designation.

Do we want to change the QMP wire format when multiple errors have been
chained together, to return a linked list or array of those errors, for
easier machine parsing of the individual errors?  If so, it requires
some documentation updates.  If not, packing the chained error
information into a single string is okay for humans, but not as nice for
computers.

> +#
>  # Since: 1.2
>  ##
>  { 'enum': 'ErrorClass',
>    'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
> -            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
> +            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap',
> +            'MultipleErrors' ] }
>  
>  ##
>  # @VersionTriple
> diff --git a/util/error.c b/util/error.c
> index b93e5c8..890ce58 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -18,28 +18,25 @@ struct Error
>  {
>      char *msg;
>      ErrorClass err_class;
> +    struct Error *next;
>  };

Merge conflicts in this area; but doesn't hold up review of the concept.

>  
>  Error *error_abort;
>  
>  static void do_error_set(Error **errp, ErrorClass err_class,
>                           void (*mod)(Error *, void *), void *mod_opaque,
> -                         const char *fmt, ...)
> +                         const char *fmt, va_list ap)
>  {
>      Error *err;
> -    va_list ap;
>      int saved_errno = errno;
>  
>      if (errp == NULL) {
>          return;
>      }
> -    assert(*errp == NULL);

Here's where I'm wondering if we can have some sort of flag to say
whether the caller was okay with error concatentation, in which case
this would be something like:

assert(!*errp || errp->chaining_okay);

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

signature.asc
Description: OpenPGP digital signature