| Message ID | 1256841750-15228-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > Makefile | 2 +- > qerror.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qerror.h | 48 ++++++++++++ > qobject.h | 1 + > 4 files changed, 290 insertions(+), 1 deletions(-) > create mode 100644 qerror.c > create mode 100644 qerror.h > > diff --git a/Makefile b/Makefile > index 813bd0a..325e583 100644 > --- a/Makefile > +++ b/Makefile > @@ -125,7 +125,7 @@ obj-y += net.o net-queue.o > obj-y += qemu-char.o aio.o net-checksum.o savevm.o > obj-y += msmouse.o ps2.o > obj-y += qdev.o qdev-properties.o > -obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o > +obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o > obj-y += qemu-config.o > > obj-$(CONFIG_BRLAPI) += baum.o > diff --git a/qerror.c b/qerror.c > new file mode 100644 > index 0000000..0359d65 > --- /dev/null > +++ b/qerror.c > @@ -0,0 +1,240 @@ > +/* > + * QError: QEMU Error data-type. > + * > + * Copyright (C) 2009 Red Hat Inc. > + * > + * Authors: > + * Luiz Capitulino <lcapitulino@redhat.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + */ > +#include "qint.h" > +#include "qjson.h" > +#include "qerror.h" > +#include "qstring.h" > +#include "sysemu.h" > +#include "qemu-common.h" > + > +static void qerror_destroy_obj(QObject *obj); > + > +static const QType qerror_type = { > + .code = QTYPE_QERROR, > + .destroy = qerror_destroy_obj, > +}; > + > +/** > + * The 'desc' member is a printf-like string, the format of the format > + * string is: > + * > + * %(KEY)TYPE > + * > + * Where KEY is a QDict key and TYPE is the type of its value, KEY and > + * its value must be passed to qerror_from_info(). > + * > + * Example: > + * > + * "foo error on device: %(device)s slot: %(slot_nr)s" > + * > + * A single percent sign can be printed if followed by a second one, > + * for example: > + * > + * "running out of foo: %(foo)d%%" > + * > + * Valid types are: > + * > + * s (string) > + * d (integer) > + */ > +static QErrorTable qerror_table[] = { > + { > + .code = QERR_UNKNOWN, > + .desc = "unknown error", > + }, > +}; > + > +/** > + * qerror_new(): Create a new QError > + * > + * Return strong reference. > + */ > +QError *qerror_new(void) > +{ > + QError *qerr; > + > + qerr = qemu_mallocz(sizeof(*qerr)); > + QOBJECT_INIT(qerr, &qerror_type); > + > + return qerr; > +} > + > +/** > + * qerror_from_info(): Create a new QError from error information > + * > + * The information consists of: > + * > + * - code: error code > + * - file: the file of where the error happend > + * - linenr: the line number of where the error happend > + * - fmt: JSON printf-like format > + * - va: va_list of all arguments > + * > + * Return strong reference. > + */ > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr, > + const char *fmt, va_list *va) > +{ > va_list doesn't need to be a pointer. Shouldn't this take a ... too? > diff --git a/qerror.h b/qerror.h > new file mode 100644 > index 0000000..2fd0d58 > --- /dev/null > +++ b/qerror.h > @@ -0,0 +1,48 @@ > +/* > + * QError header file. > + * > + * Copyright (C) 2009 Red Hat Inc. > + * > + * Authors: > + * Luiz Capitulino <lcapitulino@redhat.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + */ > +#ifndef QERROR_H > +#define QERROR_H > + > +#include <stdarg.h> > +#include "qobject.h" > + > +/* > + * IMPORTANT: errors numbers must not change after they have been > + * added here. > + */ > +typedef enum QErrorCode { > + QERR_UNKNOWN, > + QERR_MAX, > +} QErrorCode; > + > +struct QError; > + > +typedef struct QErrorTable { > + QErrorCode code; > + const char *desc; > +} QErrorTable; > + > +typedef struct QError { > + QObject_HEAD; > + int linenr; /* error line number */ > + const char *file; /* error file */ > + QObject *data; /* error specific data */ > + const QErrorTable *entry; > +} QError; > + > +QError *qerror_new(void); > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr, > + const char *fmt, va_list *va); > I don't know how I feel about the linenr/file bit. It seems prone to misused when used in a public interface (vs. automatically via a macro).
On Thu, 29 Oct 2009 15:14:12 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > Makefile | 2 +- > > qerror.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > qerror.h | 48 ++++++++++++ > > qobject.h | 1 + > > 4 files changed, 290 insertions(+), 1 deletions(-) > > create mode 100644 qerror.c > > create mode 100644 qerror.h > > > > diff --git a/Makefile b/Makefile > > index 813bd0a..325e583 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -125,7 +125,7 @@ obj-y += net.o net-queue.o > > obj-y += qemu-char.o aio.o net-checksum.o savevm.o > > obj-y += msmouse.o ps2.o > > obj-y += qdev.o qdev-properties.o > > -obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o > > +obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o > > obj-y += qemu-config.o > > > > obj-$(CONFIG_BRLAPI) += baum.o > > diff --git a/qerror.c b/qerror.c > > new file mode 100644 > > index 0000000..0359d65 > > --- /dev/null > > +++ b/qerror.c > > @@ -0,0 +1,240 @@ > > +/* > > + * QError: QEMU Error data-type. > > + * > > + * Copyright (C) 2009 Red Hat Inc. > > + * > > + * Authors: > > + * Luiz Capitulino <lcapitulino@redhat.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > + * the COPYING file in the top-level directory. > > + */ > > +#include "qint.h" > > +#include "qjson.h" > > +#include "qerror.h" > > +#include "qstring.h" > > +#include "sysemu.h" > > +#include "qemu-common.h" > > + > > +static void qerror_destroy_obj(QObject *obj); > > + > > +static const QType qerror_type = { > > + .code = QTYPE_QERROR, > > + .destroy = qerror_destroy_obj, > > +}; > > + > > +/** > > + * The 'desc' member is a printf-like string, the format of the format > > + * string is: > > + * > > + * %(KEY)TYPE > > + * > > + * Where KEY is a QDict key and TYPE is the type of its value, KEY and > > + * its value must be passed to qerror_from_info(). > > + * > > + * Example: > > + * > > + * "foo error on device: %(device)s slot: %(slot_nr)s" > > + * > > + * A single percent sign can be printed if followed by a second one, > > + * for example: > > + * > > + * "running out of foo: %(foo)d%%" > > + * > > + * Valid types are: > > + * > > + * s (string) > > + * d (integer) > > + */ > > +static QErrorTable qerror_table[] = { > > + { > > + .code = QERR_UNKNOWN, > > + .desc = "unknown error", > > + }, > > +}; > > + > > +/** > > + * qerror_new(): Create a new QError > > + * > > + * Return strong reference. > > + */ > > +QError *qerror_new(void) > > +{ > > + QError *qerr; > > + > > + qerr = qemu_mallocz(sizeof(*qerr)); > > + QOBJECT_INIT(qerr, &qerror_type); > > + > > + return qerr; > > +} > > + > > +/** > > + * qerror_from_info(): Create a new QError from error information > > + * > > + * The information consists of: > > + * > > + * - code: error code > > + * - file: the file of where the error happend > > + * - linenr: the line number of where the error happend > > + * - fmt: JSON printf-like format > > + * - va: va_list of all arguments > > + * > > + * Return strong reference. > > + */ > > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr, > > + const char *fmt, va_list *va) > > +{ > > > > va_list doesn't need to be a pointer. Ok, but if this is going to be a public interface, I think it's better to va_copy() before passing it to qobject_from_json_va() then. > Shouldn't this take a ... too? The problem is that its caller (qemu_error_structed()) also takes a ..., I don't know how to pass the arguments to a second function with variadic arguments w/o passing the va_list. > > diff --git a/qerror.h b/qerror.h > > new file mode 100644 > > index 0000000..2fd0d58 > > --- /dev/null > > +++ b/qerror.h > > @@ -0,0 +1,48 @@ > > +/* > > + * QError header file. > > + * > > + * Copyright (C) 2009 Red Hat Inc. > > + * > > + * Authors: > > + * Luiz Capitulino <lcapitulino@redhat.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > + * the COPYING file in the top-level directory. > > + */ > > +#ifndef QERROR_H > > +#define QERROR_H > > + > > +#include <stdarg.h> > > +#include "qobject.h" > > + > > +/* > > + * IMPORTANT: errors numbers must not change after they have been > > + * added here. > > + */ > > +typedef enum QErrorCode { > > + QERR_UNKNOWN, > > + QERR_MAX, > > +} QErrorCode; > > + > > +struct QError; > > + > > +typedef struct QErrorTable { > > + QErrorCode code; > > + const char *desc; > > +} QErrorTable; > > + > > +typedef struct QError { > > + QObject_HEAD; > > + int linenr; /* error line number */ > > + const char *file; /* error file */ > > + QObject *data; /* error specific data */ > > + const QErrorTable *entry; > > +} QError; > > + > > +QError *qerror_new(void); > > +QError *qerror_from_info(QErrorCode code, const char *file, int linenr, > > + const char *fmt, va_list *va); > > > > I don't know how I feel about the linenr/file bit. It seems prone to > misused when used in a public interface (vs. automatically via a macro). This is what happens in practice, qemu_error_structed() is a macro, it's where the __LINE__/__FILE__ macros are actually used. Although there is no problems in being part of a public interface, qerror_from_info() is a low-level function, it's supposed to be part of a higher level API. The only way to make this the way you suggest, would be to export qemu_error_structed() from QError someway. But I'd have to think how to do this, as we have to deal with the Monitor's bits and the QemuErrorSink API. By the way, what contributes to make this worse is the all-in-a-file Monitor module.. We also have cyclic dependencies there. I plan to split it, but probably not for 0.12.
On 10/29/2009 09:48 PM, Luiz Capitulino wrote: > > va_list doesn't need to be a pointer. > > Ok, but if this is going to be a public interface, I think it's > better to va_copy() before passing it to qobject_from_json_va() then. It is standard to pass a va_list by value without doing va_copy in the caller. >> > Shouldn't this take a ... too? > The problem is that its caller (qemu_error_structed()) also takes > a ..., I don't know how to pass the arguments to a second function with > variadic arguments w/o passing the va_list. Yeah, this is fine. Paolo
Paolo Bonzini wrote: > On 10/29/2009 09:48 PM, Luiz Capitulino wrote: > >> va_list doesn't need to be a pointer. > > > > Ok, but if this is going to be a public interface, I think it's > >better to va_copy() before passing it to qobject_from_json_va() then. > > It is standard to pass a va_list by value without doing va_copy in the > caller. That's right, just like: va_list ap; va_start(ap, format); vfprintf(stderr, format, ap); /* <- passed by value */ va_end(ap); You only need va_copy() is you're going to use some more it after passing it to the function. There should be one va_end() for every va_start() plus every va_copy(). -- Jamie
Paolo Bonzini wrote: > On 10/29/2009 09:48 PM, Luiz Capitulino wrote: >> > va_list doesn't need to be a pointer. >> >> Ok, but if this is going to be a public interface, I think it's >> better to va_copy() before passing it to qobject_from_json_va() then. > > It is standard to pass a va_list by value without doing va_copy in the > caller. > >>> > Shouldn't this take a ... too? >> The problem is that its caller (qemu_error_structed()) also takes >> a ..., I don't know how to pass the arguments to a second function with >> variadic arguments w/o passing the va_list. > > Yeah, this is fine. Usually you provide two versions of variadic functions. One that takes an ... and one that takes a va_list.
On Fri, 30 Oct 2009 08:05:57 -0500 Anthony Liguori <aliguori@us.ibm.com> wrote: > Paolo Bonzini wrote: > > On 10/29/2009 09:48 PM, Luiz Capitulino wrote: > >> > va_list doesn't need to be a pointer. > >> > >> Ok, but if this is going to be a public interface, I think it's > >> better to va_copy() before passing it to qobject_from_json_va() then. > > > > It is standard to pass a va_list by value without doing va_copy in the > > caller. > > > >>> > Shouldn't this take a ... too? > >> The problem is that its caller (qemu_error_structed()) also takes > >> a ..., I don't know how to pass the arguments to a second function with > >> variadic arguments w/o passing the va_list. > > > > Yeah, this is fine. > > Usually you provide two versions of variadic functions. One that takes > an ... and one that takes a va_list. This is what is going here, qemu_error_structed() take a ... and qobject_from_json_va() takes a va_list.
diff --git a/Makefile b/Makefile index 813bd0a..325e583 100644 --- a/Makefile +++ b/Makefile @@ -125,7 +125,7 @@ obj-y += net.o net-queue.o obj-y += qemu-char.o aio.o net-checksum.o savevm.o obj-y += msmouse.o ps2.o obj-y += qdev.o qdev-properties.o -obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o +obj-y += qint.o qstring.o qdict.o qlist.o qfloat.o qbool.o qjson.o qerror.o obj-y += qemu-config.o obj-$(CONFIG_BRLAPI) += baum.o diff --git a/qerror.c b/qerror.c new file mode 100644 index 0000000..0359d65 --- /dev/null +++ b/qerror.c @@ -0,0 +1,240 @@ +/* + * QError: QEMU Error data-type. + * + * Copyright (C) 2009 Red Hat Inc. + * + * Authors: + * Luiz Capitulino <lcapitulino@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ +#include "qint.h" +#include "qjson.h" +#include "qerror.h" +#include "qstring.h" +#include "sysemu.h" +#include "qemu-common.h" + +static void qerror_destroy_obj(QObject *obj); + +static const QType qerror_type = { + .code = QTYPE_QERROR, + .destroy = qerror_destroy_obj, +}; + +/** + * The 'desc' member is a printf-like string, the format of the format + * string is: + * + * %(KEY)TYPE + * + * Where KEY is a QDict key and TYPE is the type of its value, KEY and + * its value must be passed to qerror_from_info(). + * + * Example: + * + * "foo error on device: %(device)s slot: %(slot_nr)s" + * + * A single percent sign can be printed if followed by a second one, + * for example: + * + * "running out of foo: %(foo)d%%" + * + * Valid types are: + * + * s (string) + * d (integer) + */ +static QErrorTable qerror_table[] = { + { + .code = QERR_UNKNOWN, + .desc = "unknown error", + }, +}; + +/** + * qerror_new(): Create a new QError + * + * Return strong reference. + */ +QError *qerror_new(void) +{ + QError *qerr; + + qerr = qemu_mallocz(sizeof(*qerr)); + QOBJECT_INIT(qerr, &qerror_type); + + return qerr; +} + +/** + * qerror_from_info(): Create a new QError from error information + * + * The information consists of: + * + * - code: error code + * - file: the file of where the error happend + * - linenr: the line number of where the error happend + * - fmt: JSON printf-like format + * - va: va_list of all arguments + * + * Return strong reference. + */ +QError *qerror_from_info(QErrorCode code, const char *file, int linenr, + const char *fmt, va_list *va) +{ + QError *qerr; + + assert((code > 0) && (code < QERR_MAX)); + + qerr = qerror_new(); + qerr->entry = &qerror_table[code]; + qerr->file = file; + qerr->linenr = linenr; + if (fmt) { + qerr->data = qobject_from_json_va(fmt, va); + assert(qerr->data != NULL); + } + + return qerr; +} + +static char *get_substr(const char *start, const char *end) +{ + char *str; + size_t length; + + length = end - start + 1; + str = qemu_malloc(length + 1); + memcpy(str, start, length); + str[length] = '\0'; + + return str; +} + +static void qerror_abort(const QError *qerror) +{ + fprintf(stderr, " in '%s'\n", qerror->entry->desc); + abort(); +} + +#define ERROR_PREFIX "\n\nqerror: " + +static void type_error(const QError *qerror, int c) +{ + fprintf(stderr, ERROR_PREFIX "invalid type '%c'", c); + qerror_abort(qerror); +} + +static void key_error(const QError *qerror, const char *key) +{ + fprintf(stderr, ERROR_PREFIX "key '%s' not found in QDict ", key); + fprintf(stderr, "check call at %s:%d\n", qerror->file, qerror->linenr); + abort(); +} + +static void parse_error(const QError *qerror, int c) +{ + fprintf(stderr, ERROR_PREFIX "expected '%c'", c); + qerror_abort(qerror); +} + +static const char *print_field(const QError *qerror, const char *start) +{ + int type; + QDict *qdict; + char *name; + const char *end; + + if (*start != '%') + parse_error(qerror, '%'); + start++; + if (*start != '(') + parse_error(qerror, '('); + start++; + + end = strchr(start, ')'); + if (!end) + parse_error(qerror, ')'); + + name = get_substr(start, end - 1); + qdict = qobject_to_qdict(qerror->data); + + if (!qdict_haskey(qdict, name)) { + key_error(qerror, name); + } + + type = *++end; + switch (type) { + case 's': + qemu_error("%s", qdict_get_str(qdict, name)); + break; + case 'd': + qemu_error("%" PRId64, qdict_get_int(qdict, name)); + break; + default: + type_error(qerror, type); + } + + qemu_free(name); + return ++end; +} + +/** + * qerror_print(): Print QError data + * + * This function will print the member 'desc' of the specified QError object, + * it uses qemu_error() for this, so that the output is routed to the right + * place (ie. stderr ou Monitor's device). + */ +void qerror_print(const QError *qerror) +{ + const char *p; + + for (p = qerror->entry->desc; *p != '\0'; p++) { + if (*p == '%') { + if (*(p + 1) == '%') { + p++; + } else { + p = print_field(qerror, p); + if (*p == '\0') { + break; + } + } + } + + /* + * FIXME: This is inefficient, qemu_error() likes strings + */ + qemu_error("%c", *p); + } + + qemu_error("\n"); +} + +/** + * qobject_to_qerror(): Convert a QObject into a QError + */ +QError *qobject_to_qerror(const QObject *obj) +{ + if (qobject_type(obj) != QTYPE_QERROR) { + return NULL; + } + + return container_of(obj, QError, base); +} + +/** + * qerror_destroy_obj(): Free all memory allocated by a QError + */ +static void qerror_destroy_obj(QObject *obj) +{ + QError *qerr; + + assert(obj != NULL); + qerr = qobject_to_qerror(obj); + + qobject_decref(qerr->data); + qemu_free(qerr); +} diff --git a/qerror.h b/qerror.h new file mode 100644 index 0000000..2fd0d58 --- /dev/null +++ b/qerror.h @@ -0,0 +1,48 @@ +/* + * QError header file. + * + * Copyright (C) 2009 Red Hat Inc. + * + * Authors: + * Luiz Capitulino <lcapitulino@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + */ +#ifndef QERROR_H +#define QERROR_H + +#include <stdarg.h> +#include "qobject.h" + +/* + * IMPORTANT: errors numbers must not change after they have been + * added here. + */ +typedef enum QErrorCode { + QERR_UNKNOWN, + QERR_MAX, +} QErrorCode; + +struct QError; + +typedef struct QErrorTable { + QErrorCode code; + const char *desc; +} QErrorTable; + +typedef struct QError { + QObject_HEAD; + int linenr; /* error line number */ + const char *file; /* error file */ + QObject *data; /* error specific data */ + const QErrorTable *entry; +} QError; + +QError *qerror_new(void); +QError *qerror_from_info(QErrorCode code, const char *file, int linenr, + const char *fmt, va_list *va); +void qerror_print(const QError *qerror); +QError *qobject_to_qerror(const QObject *obj); + +#endif /* QERROR_H */ diff --git a/qobject.h b/qobject.h index 167b607..838dddc 100644 --- a/qobject.h +++ b/qobject.h @@ -43,6 +43,7 @@ typedef enum { QTYPE_QLIST, QTYPE_QFLOAT, QTYPE_QBOOL, + QTYPE_QERROR, } qtype_code; struct QObject;
QError is a high-level data type that can be used to store the following error information: o Error data: Any kind of data generated at error time can be stored (if turned into a QObject) o Description: A string description, which may contain error data o Error location: file name and line number of where the error was triggered Before using it functions usually have to register a new error type, this is done by adding a new entry in the qerror_type[] table with the following error information: 1. Code, which should be added to enum QErrorCode first 2. Description, which is a printf-like string QError exports the following functions: - qerror_new(): Create a new 'empty' QError - qerror_from_va(): Create a new QError from the specified va_list - qerror_print(): Print the specified QError Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- Makefile | 2 +- qerror.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qerror.h | 48 ++++++++++++ qobject.h | 1 + 4 files changed, 290 insertions(+), 1 deletions(-) create mode 100644 qerror.c create mode 100644 qerror.h