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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 01 Mar 2010 10:19:22 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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).
> 
> Yes, I am pretty sure.
> 
> Chasing list pointers by hand for the millionth time sucks, and that's
> why we have qemu-queue.h.  But we're not chasing pointers here.  We're
> threading together a stack of current locations, and it takes us all of
> five trivial assignments (two in loc_push_restore(), one in
> loc_push_none(), two in loc_pop()).  Eight if you count loc_save() and
> loc_restore().
> 
> I tried to sketch how the same thing would look using sys-queue.h, and
> gave up because it made my head hurt.
> 
> >> +
> >> +/*
> >> + * 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().
> 
> What exactly would you like me to document?

 The format of the output, specially the automatic stuff (which is
probably only worth for qemu_error()).