On Wed, 24 Feb 2010 18:55:29 +0100 Markus Armbruster <arm...@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 <arm...@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); > > /*