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