Re: [Qemu-devel] [PATCH 07/10] Introduce QError
Luiz Capitulino writes:
> On Wed, 18 Nov 2009 16:16:11 +0100
> Markus Armbruster wrote:
>
>> Luiz Capitulino writes:
>
> [...]
>
>> > +static const char *append_field(QString *outstr, const QError *qerror,
>> > +const char *start)
>> > +{
>> > +QObject *obj;
>> > +QDict *qdict;
>> > +QString *key_qs;
>> > +const char *end, *key;
>> > +
>> > +if (*start != '%')
>> > +parse_error(qerror, '%');
>>
>> Can't happen, because it gets called only with *start == '%'. Taking
>> pointer to the character following the '%' as argument would sidestep
>> the issue. But I'm fine with leaving it as is.
>
> It's just an assertion.
It's not coded as an assertion. If we ever do coverage testing, it'll
stick out. But again, I'm fine with it.
>> > +start++;
>> > +if (*start != '(')
>> > +parse_error(qerror, '(');
>> > +start++;
>> > +
>> > +end = strchr(start, ')');
>> > +if (!end)
>> > +parse_error(qerror, ')');
>> > +
>> > +key_qs = qstring_from_substr(start, 0, end - start - 1);
>> > +key = qstring_get_str(key_qs);
>> > +
>> > +qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
>> > +obj = qdict_get(qdict, key);
>> > +if (!obj) {
>> > +qerror_abort(qerror, "key '%s' not found in QDict", key);
>> > +}
>> > +
>> > +switch (qobject_type(obj)) {
>> > +case QTYPE_QSTRING:
>> > +qstring_append(outstr, qdict_get_str(qdict, key));
>> > +break;
>> > +case QTYPE_QINT:
>> > +qstring_append_int(outstr, qdict_get_int(qdict, key));
>> > +break;
>> > +default:
>> > +qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
>> > +}
>> > +
>> > +QDECREF(key_qs);
>>
>> Looks like you create key_qs just because it's a convenient way to
>> extract key zero-terminated. Correct?
>
> Yes, as a substring of 'desc', which is passed through 'start'.
Funny that the convenient way to extract a substring is to go through
QString. Fine with me.
> [...]
>
>> > diff --git a/qjson.c b/qjson.c
>> > index 12e6cf0..60c904d 100644
>> > --- a/qjson.c
>> > +++ b/qjson.c
>> > @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
>> > }
>> > break;
>> > }
>> > +case QTYPE_QERROR:
>> > +/* XXX: should QError be emitted? */
>>
>> Pros & cons?
>
> It's probably convenient to have qjson emitting QError, I'm unsure
> if we should do that for all kinds of QObjects though.
For a general purpose system, I'd recommend to cover all types. But as
long as this has just one user (QEMU), it can use the special purpose
excuse not to.
>> > case QTYPE_NONE:
>> > break;
>> > }
>> > diff --git a/qobject.h b/qobject.h
>> > index 2270ec1..07de211 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;
>>
>> Erroneous QERRs are detected only when they're passed to
>> qerror_from_info() at run-time, i.e. when the error happens. Likewise
>> for erroneous qerror_table[].desc. Perhaps a unit test to ensure
>> qerror_table[] is sane would make sense. Can't protect from passing
>> unknown errors to qerror_from_info(), but that shouldn't be a problem in
>> practice.
>
> We could also have a debug function that could run once at startup
> and do the check.
Yes.
Re: [Qemu-devel] [PATCH 07/10] Introduce QError
On Wed, 18 Nov 2009 13:58:17 -0600 Anthony Liguori wrote: > Daniel P. Berrange wrote: > > On Tue, Nov 17, 2009 at 05:43:54PM -0200, Luiz Capitulino wrote: > > > >> QError is a high-level data type which represents an exception > >> in QEMU, it stores the following error information: > >> > >> - class Error class name (eg. "ServiceUnavailable") > >> - descriptionA detailed error description, which can contain > >> references to run-time error data > >> - filename The file name of where the error occurred > >> - line numberThe exact line number of the error > >> > > > > If we're going to collect these two, then also add in the function > > name, since that's typically more useful than filename/line number > > alone. > > > > I'm not convinced it's a good idea to put that info on the wire. It's > unstable across any build of qemu. However, since it's extra info, it > doesn't bother me that much if people think it's useful for debugging > purposes. It's really for debugging, so that we can have a detailed error description when the error macro has a wrong syntax. That said, we could have a compile time switch to activate extra debugging information on the wire. But that's a brainstorm.
Re: [Qemu-devel] [PATCH 07/10] Introduce QError
Daniel P. Berrange wrote: On Tue, Nov 17, 2009 at 05:43:54PM -0200, Luiz Capitulino wrote: QError is a high-level data type which represents an exception in QEMU, it stores the following error information: - class Error class name (eg. "ServiceUnavailable") - descriptionA detailed error description, which can contain references to run-time error data - filename The file name of where the error occurred - line numberThe exact line number of the error If we're going to collect these two, then also add in the function name, since that's typically more useful than filename/line number alone. I'm not convinced it's a good idea to put that info on the wire. It's unstable across any build of qemu. However, since it's extra info, it doesn't bother me that much if people think it's useful for debugging purposes. Regards, Daniel -- Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH 07/10] Introduce QError
On Tue, Nov 17, 2009 at 05:43:54PM -0200, Luiz Capitulino wrote: > QError is a high-level data type which represents an exception > in QEMU, it stores the following error information: > > - class Error class name (eg. "ServiceUnavailable") > - descriptionA detailed error description, which can contain > references to run-time error data > - filename The file name of where the error occurred > - line numberThe exact line number of the error If we're going to collect these two, then also add in the function name, since that's typically more useful than filename/line number alone. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Re: [Qemu-devel] [PATCH 07/10] Introduce QError
On Wed, 18 Nov 2009 16:16:11 +0100
Markus Armbruster wrote:
> Luiz Capitulino writes:
[...]
> > +static const char *append_field(QString *outstr, const QError *qerror,
> > +const char *start)
> > +{
> > +QObject *obj;
> > +QDict *qdict;
> > +QString *key_qs;
> > +const char *end, *key;
> > +
> > +if (*start != '%')
> > +parse_error(qerror, '%');
>
> Can't happen, because it gets called only with *start == '%'. Taking
> pointer to the character following the '%' as argument would sidestep
> the issue. But I'm fine with leaving it as is.
It's just an assertion.
> > +start++;
> > +if (*start != '(')
> > +parse_error(qerror, '(');
> > +start++;
> > +
> > +end = strchr(start, ')');
> > +if (!end)
> > +parse_error(qerror, ')');
> > +
> > +key_qs = qstring_from_substr(start, 0, end - start - 1);
> > +key = qstring_get_str(key_qs);
> > +
> > +qdict = qobject_to_qdict(qdict_get(qerror->error, "data"));
> > +obj = qdict_get(qdict, key);
> > +if (!obj) {
> > +qerror_abort(qerror, "key '%s' not found in QDict", key);
> > +}
> > +
> > +switch (qobject_type(obj)) {
> > +case QTYPE_QSTRING:
> > +qstring_append(outstr, qdict_get_str(qdict, key));
> > +break;
> > +case QTYPE_QINT:
> > +qstring_append_int(outstr, qdict_get_int(qdict, key));
> > +break;
> > +default:
> > +qerror_abort(qerror, "invalid type '%c'", qobject_type(obj));
> > +}
> > +
> > +QDECREF(key_qs);
>
> Looks like you create key_qs just because it's a convenient way to
> extract key zero-terminated. Correct?
Yes, as a substring of 'desc', which is passed through 'start'.
[...]
> > diff --git a/qjson.c b/qjson.c
> > index 12e6cf0..60c904d 100644
> > --- a/qjson.c
> > +++ b/qjson.c
> > @@ -224,6 +224,8 @@ static void to_json(const QObject *obj, QString *str)
> > }
> > break;
> > }
> > +case QTYPE_QERROR:
> > +/* XXX: should QError be emitted? */
>
> Pros & cons?
It's probably convenient to have qjson emitting QError, I'm unsure
if we should do that for all kinds of QObjects though.
> > case QTYPE_NONE:
> > break;
> > }
> > diff --git a/qobject.h b/qobject.h
> > index 2270ec1..07de211 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;
>
> Erroneous QERRs are detected only when they're passed to
> qerror_from_info() at run-time, i.e. when the error happens. Likewise
> for erroneous qerror_table[].desc. Perhaps a unit test to ensure
> qerror_table[] is sane would make sense. Can't protect from passing
> unknown errors to qerror_from_info(), but that shouldn't be a problem in
> practice.
We could also have a debug function that could run once at startup
and do the check.
Re: [Qemu-devel] [PATCH 07/10] Introduce QError
Luiz Capitulino writes:
> QError is a high-level data type which represents an exception
> in QEMU, it stores the following error information:
>
> - class Error class name (eg. "ServiceUnavailable")
> - descriptionA detailed error description, which can contain
> references to run-time error data
> - filename The file name of where the error occurred
> - line numberThe exact line number of the error
> - run-time data Any run-time error data
>
> This commit adds the basic interface plus two error classes, one
> for 'device not found' errors and another one for 'service unavailable'
> errors.
I'd rather add error classes in the first commit using them.
> Signed-off-by: Luiz Capitulino
> ---
> Makefile |2 +-
> qerror.c | 265
> +
> qerror.h | 46 +++
> qjson.c |2 +
> qobject.h |1 +
> 5 files changed, 315 insertions(+), 1 deletions(-)
> create mode 100644 qerror.c
> create mode 100644 qerror.h
>
> diff --git a/Makefile b/Makefile
> index d770e2a..c0b65b6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -138,7 +138,7 @@ obj-y += qemu-char.o aio.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 json-lexer.o
> -obj-y += json-streamer.o json-parser.o qjson.o
> +obj-y += json-streamer.o json-parser.o qjson.o qerror.o
> obj-y += qemu-config.o block-migration.o
>
> obj-$(CONFIG_BRLAPI) += baum.o
> diff --git a/qerror.c b/qerror.c
> new file mode 100644
> index 000..beb215d
> --- /dev/null
> +++ b/qerror.c
> @@ -0,0 +1,265 @@
> +/*
> + * QError: QEMU Error data-type.
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * Authors:
> + * Luiz Capitulino
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +#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' parameter is a printf-like string, the format of the format
> + * string is:
> + *
> + * %(KEY)
> + *
> + * Where KEY is a QDict key, which has to be passed to qerror_from_info().
> + *
> + * Example:
> + *
> + * "foo error on device: %(device) slot: %(slot_nr)"
> + *
> + * A single percent sign can be printed if followed by a second one,
> + * for example:
> + *
> + * "running out of foo: %(foo)%%"
> + */
> +const QErrorStringTable qerror_table[] = {
> +{
> +.error_fmt = QERR_DEVICE_NOT_FOUND,
> +.desc= "device \"%(name)\" not found",
> +},
> +{
> +.error_fmt = QERR_SERVICE_UNAVAILABLE,
> +.desc= "%(reason)",
> +},
> +{}
> +};
> +
> +/**
> + * 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;
> +}
> +
> +static void qerror_abort(const QError *qerr, const char *fmt, ...)
> +{
> +va_list ap;
> +
> +fprintf(stderr, "qerror: ");
> +
> +va_start(ap, fmt);
> +vfprintf(stderr, fmt, ap);
> +va_end(ap);
> +
> +fprintf(stderr, " - call at %s:%d\n", qerr->file, qerr->linenr);
> +abort();
> +}
> +
> +static void qerror_set_data(QError *qerr, const char *fmt, va_list *va)
> +{
> +QObject *obj;
> +
> +obj = qobject_from_jsonv(fmt, va);
> +if (!obj) {
> +qerror_abort(qerr, "invalid format '%s'", fmt);
> +}
> +if (qobject_type(obj) != QTYPE_QDICT) {
> +qerror_abort(qerr, "error format is not a QDict '%s'", fmt);
> +}
> +
> +qerr->error = qobject_to_qdict(obj);
> +
> +obj = qdict_get(qerr->error, "class");
> +if (!obj) {
> +qerror_abort(qerr, "missing 'class' key in '%s'", fmt);
> +}
> +if (qobject_type(obj) != QTYPE_QSTRING) {
> +qerror_abort(qerr, "'class' key value should be a QString");
> +}
> +
> +obj = qdict_get(qerr->error, "data");
> +if (!obj) {
> +qerror_abort(qerr, "missing 'data' key in '%s'", fmt);
> +}
> +if (qobject_type(obj) != QTYPE_QDICT) {
> +qerror_abort(qerr, "'data' key value should be a QDICT");
> +}
> +}
> +
> +static void qerror_set_desc(const char *fmt, QError *qerr)
> +{
> +int i;
> +
> +// FIXME: inefficient loop
> +
> +for (i = 0; qerror_table[i].error_fmt; i++) {
> +if (strcmp(qerror_table[i].error_fmt, fmt) == 0) {
> +qerr->entry = &qerror_table[i];
> +return;
> +}
> +}
> +
> +qerror_abort(qerr, "error format '%s' not found", fmt);
> +}
> +
> +/**
> + * qerror_from_info(): Create a new QError f
