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