On Mon, 01 Mar 2010 10:19:22 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > 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). > > 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()).