Re: [Qemu-devel] [RFC PATCH v1 05/25] error: Add error prefix API

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

On 09/10/2015 11:33 PM, Peter Crosthwaite wrote:
> Add an API to prefix an already set error with a caller-centric
> message.
> 
> If multiple errors are set, all are prefixed individually.
> 

Might be nice to rebase your series to add this first, prior to
multi-error support.  (That is, while prefixing multiple errors requires
multi-error support, adding caller-centric prefix might be useful even
now without multi-error).

> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 
>  include/qapi/error.h |  6 ++++++
>  util/error.c         | 26 ++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f44c451..b25c72f 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -78,6 +78,12 @@ ErrorClass error_get_class(const Error *err);
>  Error *error_copy(const Error *err);
>  
>  /**
> + * Prefix an error message with a formatted string.
> + */
> +
> +void error_prefix(Error *err, const char *fmt, ...);

The string is basically only adding details for human consumption.

> +++ b/util/error.c
> @@ -19,6 +19,7 @@ struct Error
>      char *msg;
>      ErrorClass err_class;
>      struct Error *next;
> +    bool prefixed;
>  };

I'm not sure I follow why this field is needed.

>  
>  Error *error_abort;
> @@ -142,6 +143,26 @@ const char *error_get_pretty(Error *err)
>      return err->msg;
>  }
>  
> +void error_prefix(Error *err, const char *fmt, ...) {

Brace on its own line.

> +    char *msg;
> +    char *fmt_full;
> +    va_list ap;
> +
> +    if (!err || err->prefixed) {
> +        return;
> +    }
> +    err->prefixed = true;
> +
> +    msg = err->msg;
> +    fmt_full =  g_strdup_printf("%s%%s", fmt);
> +
> +    va_start(ap, fmt);
> +    err->msg = g_strdup_printf(fmt_full, ap, msg);

I don't think that is portable.  Passing va_list as an argument to plain
printf just isn't going to do what you think.
(There has been a request to add recursive printf via %r,
http://austingroupbugs.net/view.php?id=800, but glibc does not support
the extension yet)

It's not to say that you can't chain, just that you can't chain like
this.  I don't know if using GString internally to error would make the
matter any easier (but one of the patches that will probably land before
yours, and thus affect your need to rebase, is mine which adds use of
GString for adding a human-readable SUFFIX rather than prefix:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02818.html)

> +    va_end(ap);
> +    g_free(fmt_full);
> +    g_free(msg);
> +}
> +
>  void error_report_err(Error *err)
>  {
>      error_report("%s", error_get_pretty(err));
> @@ -170,6 +191,11 @@ void error_propagate(Error **dst_errp, Error *local_err)
>  
>          *dst_errp = local_err;
>          for (i = local_err; i; i = i->next) {
> +            /* Propagation implies that the caller is no longer the owner of the
> +             * error. Therefore reset prefixes, so higher level handlers can
> +             * prefix again.
> +             */
> +            i->prefixed = false;

I'm still not quite sure I buy the semantics of this flag.

>              dst_errp = &i->next;
>          }
>          *dst_errp = old_dst_err;
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]