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