Patchwork [RFC,17/48] error: Infrastructure to track locations for error reporting

login
register
mail settings
Submitter Markus Armbruster
Date Feb. 24, 2010, 5:55 p.m.
Message ID <1267034160-3517-18-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/46147/
State New
Headers show

Comments

Markus Armbruster - Feb. 24, 2010, 5:55 p.m.
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.

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(-)
Luiz Capitulino - Feb. 26, 2010, 7:45 p.m.
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);
>  
>  /*
Markus Armbruster - March 1, 2010, 9:19 a.m.
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?  That qerror_print() doesn't
ignore the location stored in its argument?  Probably not.  That
qemu_error_new() uses the current location just like 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);
>>  
>>  /*
Luiz Capitulino - March 1, 2010, 4:15 p.m.
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()).

Patch

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;
+}
+
+/*
+ * 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;
+    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);
 }
 
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);
 
 /*