Re: [Qemu-devel] [PATCH RFC 17/48] error: Infrastructure to track locations for error reporting

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 24 Feb 2010 18:55:29 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> New struct Location holds a location.  So far, the only location is
> LOC_NONE, so this doesn't do anything useful yet.
> 
> Passing the current location all over the place would be too
> cumbersome.  Hide it away in static cur_loc instead, and provide
> accessors.  Print it in qemu_error().
> 
> Store it in QError, and print it in qerror_print().
> 
> Store it in QemuOpt, for use by qemu_opts_foreach().  This makes
> qemu_error() do the right thing when it runs within
> qemu_opts_foreach().
> 
> We may still have to store it in other data structures holding user
> input for better error messages.  Left for another day.

 General implementation looks good to me, minor comments follow.

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qemu-error.c  |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-error.h  |   16 +++++++++++
>  qemu-option.c |    7 +++++
>  qemu-tool.c   |   24 ++++++++++++++++
>  qerror.c      |    5 +++-
>  qerror.h      |    4 ++-
>  6 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-error.c b/qemu-error.c
> index 075c64d..0778001 100644
> --- a/qemu-error.c
> +++ b/qemu-error.c
> @@ -41,10 +41,93 @@ void error_printf(const char *fmt, ...)
>      va_end(ap);
>  }
>  
> +static Location std_loc = {
> +    .kind = LOC_NONE
> +};
> +static Location *cur_loc = &std_loc;
> +
> +/*
> + * Push location saved in LOC onto the location stack, return it.
> + * The top of that stack is the current location.
> + * Needs a matching loc_pop().
> + */
> +Location *loc_push_restore(Location *loc)
> +{
> +    assert(!loc->prev);
> +    loc->prev = cur_loc;
> +    cur_loc = loc;
> +    return loc;
> +}
> +
> +/*
> + * Initialize *LOC to "nowhere", push it onto the location stack.
> + * The top of that stack is the current location.
> + * Needs a matching loc_pop().
> + * Return LOC.
> + */
> +Location *loc_push_none(Location *loc)
> +{
> +    loc->kind = LOC_NONE;
> +    loc->prev = NULL;
> +    return loc_push_restore(loc);
> +}
> +
> +/*
> + * Pop the location stack.
> + * LOC must be the current location, i.e. the top of the stack.
> + */
> +Location *loc_pop(Location *loc)
> +{
> +    assert(cur_loc == loc && loc->prev);
> +    cur_loc = loc->prev;
> +    loc->prev = NULL;
> +    return loc;
> +}
> +
> +/*
> + * Save the current location in LOC, return LOC.
> + */
> +Location *loc_save(Location *loc)
> +{
> +    *loc = *cur_loc;
> +    loc->prev = NULL;
> +    return loc;
> +}
> +
> +/*
> + * Change the current location to the one saved in LOC.
> + */
> +void loc_restore(Location *loc)
> +{
> +    Location *prev = cur_loc->prev;
> +    assert(!loc->prev);
> +    *cur_loc = *loc;
> +    cur_loc->prev = prev;
> +}

 Are you sure that using a established interface like qemu-queue.h
isn't better? Maybe we could add an API for stacks.. We have stack
support in QList, but then Location would have to be a QObject (not
sure if it's worth it).

> +
> +/*
> + * Change the current location to "nowhere in particular".
> + */
> +void loc_set_none(void)
> +{
> +    cur_loc->kind = LOC_NONE;
> +}
> +
> +/*
> + * Print current location to current monitor if we have one, else to stderr.
> + */
> +void error_print_loc(void)
> +{
> +    switch (cur_loc->kind) {
> +    default: ;
> +    }
> +}
> +
>  void qemu_error(const char *fmt, ...)
>  {
>      va_list ap;
>  
> +    error_print_loc();
>      va_start(ap, fmt);
>      error_vprintf(fmt, ap);
>      va_end(ap);
> diff --git a/qemu-error.h b/qemu-error.h
> index d90f1da..ebf4bf9 100644
> --- a/qemu-error.h
> +++ b/qemu-error.h
> @@ -13,8 +13,24 @@
>  #ifndef QEMU_ERROR_H
>  #define QEMU_ERROR_H
>  
> +typedef struct Location {
> +    /* all members are private to qemu-error.c */
> +    enum { LOC_NONE } kind;
> +    int n;

 Better to choose a better name for 'n'.

> +    const void *ptr;
> +    struct Location *prev;
> +} Location;
> +
> +Location *loc_push_restore(Location *loc);
> +Location *loc_push_none(Location *loc);
> +Location *loc_pop(Location *loc);
> +Location *loc_save(Location *loc);
> +void loc_restore(Location *loc);
> +void loc_set_none(void);
> +
>  void error_vprintf(const char *fmt, va_list ap);
>  void error_printf(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
> +void error_print_loc(void);
>  void qemu_error(const char *fmt, ...) __attribute__ ((format(printf, 1, 2)));
>  void qemu_error_internal(const char *file, int linenr, const char *func,
>                           const char *fmt, ...)
> diff --git a/qemu-option.c b/qemu-option.c
> index de40bff..ab488e4 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -27,6 +27,7 @@
>  #include <string.h>
>  
>  #include "qemu-common.h"
> +#include "qemu-error.h"
>  #include "qemu-option.h"
>  
>  /*
> @@ -483,6 +484,7 @@ struct QemuOpt {
>  struct QemuOpts {
>      char *id;
>      QemuOptsList *list;
> +    Location loc;
>      QTAILQ_HEAD(QemuOptHead, QemuOpt) head;
>      QTAILQ_ENTRY(QemuOpts) next;
>  };
> @@ -653,6 +655,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exist
>          opts->id = qemu_strdup(id);
>      }
>      opts->list = list;
> +    loc_save(&opts->loc);
>      QTAILQ_INIT(&opts->head);
>      QTAILQ_INSERT_TAIL(&list->head, opts, next);
>      return opts;
> @@ -810,13 +813,17 @@ int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc)
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>                        int abort_on_failure)
>  {
> +    Location loc;
>      QemuOpts *opts;
>      int rc = 0;
>  
> +    loc_push_none(&loc);
>      QTAILQ_FOREACH(opts, &list->head, next) {
> +        loc_restore(&opts->loc);
>          rc |= func(opts, opaque);
>          if (abort_on_failure  &&  rc != 0)
>              break;
>      }
> +    loc_pop(&loc);
>      return rc;
>  }
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 26f46eb..4bbd9e6 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -104,6 +104,30 @@ int64_t qemu_get_clock(QEMUClock *clock)
>      return (tv.tv_sec * 1000000000LL + (tv.tv_usec * 1000)) / 1000000;
>  }
>  
> +Location *loc_push_restore(Location *loc)
> +{
> +    return loc;
> +}
> +
> +Location *loc_push_none(Location *loc)
> +{
> +    return loc;
> +}
> +
> +Location *loc_pop(Location *loc)
> +{
> +    return loc;
> +}
> +
> +Location *loc_save(Location *loc)
> +{
> +    return loc;
> +}
> +
> +void loc_restore(Location *loc)
> +{
> +}
> +
>  void qemu_error(const char *fmt, ...)
>  {
>      va_list args;
> diff --git a/qerror.c b/qerror.c
> index c09c3b8..92b05eb 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -224,6 +224,7 @@ QError *qerror_from_info(const char *file, int linenr, const char *func,
>      QError *qerr;
>  
>      qerr = qerror_new();
> +    loc_save(&qerr->loc);
>      qerr->linenr = linenr;
>      qerr->file = file;
>      qerr->func = func;
> @@ -321,10 +322,12 @@ QString *qerror_human(const QError *qerror)
>   * it uses qemu_error() for this, so that the output is routed to the right
>   * place (ie. stderr or Monitor's device).
>   */
> -void qerror_print(const QError *qerror)
> +void qerror_print(QError *qerror)
>  {
>      QString *qstring = qerror_human(qerror);
> +    loc_push_restore(&qerror->loc);
>      qemu_error("%s", qstring_get_str(qstring));
> +    loc_pop(&qerror->loc);
>      QDECREF(qstring);
>  }

 The new behavior should be documented here and/or in qemu_error().

>  
> diff --git a/qerror.h b/qerror.h
> index ee59615..f25be1b 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -14,6 +14,7 @@
>  
>  #include "qdict.h"
>  #include "qstring.h"
> +#include "qemu-error.h"
>  #include <stdarg.h>
>  
>  typedef struct QErrorStringTable {
> @@ -24,6 +25,7 @@ typedef struct QErrorStringTable {
>  typedef struct QError {
>      QObject_HEAD;
>      QDict *error;
> +    Location loc;
>      int linenr;
>      const char *file;
>      const char *func;
> @@ -34,7 +36,7 @@ QError *qerror_new(void);
>  QError *qerror_from_info(const char *file, int linenr, const char *func,
>                           const char *fmt, va_list *va);
>  QString *qerror_human(const QError *qerror);
> -void qerror_print(const QError *qerror);
> +void qerror_print(QError *qerror);
>  QError *qobject_to_qerror(const QObject *obj);
>  
>  /*