Re: [Qemu-devel] [PATCH 07/10] Introduce QError

[Markus Armbruster]

Luiz Capitulino <address@hidden> 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")
> - description    A detailed error description, which can contain
>                  references to run-time error data
> - filename       The file name of where the error occurred
> - line number    The 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 <address@hidden>
> ---
>  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 0000000..beb215d
> --- /dev/null
> +++ b/qerror.c
> @@ -0,0 +1,265 @@
> +/*
> + * QError: QEMU Error data-type.
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <address@hidden>
> + *
> + * 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 from error information
> + *
> + * The information consists of:
> + *
> + * - file   the file name of where the error occurred
> + * - linenr the line number of where the error occurred
> + * - fmt    JSON printf-like dictionary, there must exist keys 'class' and
> + *          'data'
> + * - va     va_list of all arguments specified by fmt
> + *
> + * Return strong reference.
> + */
> +QError *qerror_from_info(const char *file, int linenr, const char *fmt,
> +                         va_list *va)
> +{
> +    QError *qerr;
> +
> +    qerr = qerror_new();
> +    qerr->linenr = linenr;
> +    qerr->file = file;
> +
> +    if (!fmt) {
> +        qerror_abort(qerr, "QDict not specified");
> +    }
> +
> +    qerror_set_data(qerr, fmt, va);
> +    qerror_set_desc(fmt, qerr);

Recommend to have both functions take qerr, fmt in the same order.
Since they both update qerr, I'd put qerr on the left.

> +
> +    return qerr;
> +}
> +
> +static void parse_error(const QError *qerror, int c)
> +{
> +    qerror_abort(qerror, "expected '%c' in '%s'", c, qerror->entry->desc);
> +}
> +
> +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.

> +    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?

> +    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).

s/ ou / or /

> + */
> +void qerror_print(const QError *qerror)
> +{
> +    const char *p;
> +    QString *qstring;
> +
> +    assert(qerror->entry != NULL);
> +
> +    qstring = qstring_new();
> +
> +    for (p = qerror->entry->desc; *p != '\0';) {
> +        if (*p != '%') {
> +            qstring_append_chr(qstring, *p++);
> +        } else if (*(p + 1) == '%') {
> +            qstring_append_chr(qstring, '%');
> +            p += 2;
> +        } else {
> +            p = append_field(qstring, qerror, p);
> +        }
> +    }
> +
> +    qemu_error("%s\n", qstring_get_str(qstring));
> +    QDECREF(qstring);
> +}
> +
> +/**
> + * 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);
> +
> +    QDECREF(qerr->error);
> +    qemu_free(qerr);
> +}
> diff --git a/qerror.h b/qerror.h
> new file mode 100644
> index 0000000..d262863
> --- /dev/null
> +++ b/qerror.h
> @@ -0,0 +1,46 @@
> +/*
> + * QError header file.
> + *
> + * Copyright (C) 2009 Red Hat Inc.
> + *
> + * Authors:
> + *  Luiz Capitulino <address@hidden>
> + *
> + * 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.
> + */
> +#ifndef QERROR_H
> +#define QERROR_H
> +
> +#include "qdict.h"
> +#include <stdarg.h>
> +
> +typedef struct QErrorStringTable {
> +    const char *desc;
> +    const char *error_fmt;
> +} QErrorStringTable;
> +
> +typedef struct QError {
> +    QObject_HEAD;
> +    QDict *error;
> +    int linenr;
> +    const char *file;
> +    const QErrorStringTable *entry;
> +} QError;
> +
> +QError *qerror_new(void);
> +QError *qerror_from_info(const char *file, int linenr, const char *fmt,
> +                         va_list *va);
> +void qerror_print(const QError *qerror);
> +QError *qobject_to_qerror(const QObject *obj);
> +
> +/*
> + * QError class list
> + */
> +#define QERR_DEVICE_NOT_FOUND \
> +        "{ 'class': 'DeviceNotFound', 'data': { 'name': %s } }"
> +
> +#define QERR_SERVICE_UNAVAILABLE \
> +        "{ 'class': 'ServiceUnavailable', 'data': { 'reason': %s } }"
> +
> +#endif /* QERROR_H */
> 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?

>      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.