[07/10] Introduce QError

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.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 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

Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> ---
>  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 <lcapitulino@redhat.com>
> + *
> + * 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 <lcapitulino@redhat.com>
> + *
> + * 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.

On Wed, 18 Nov 2009 16:16:11 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> 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.

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")
> - 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

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

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")
>> - 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
>>     
>
> 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
>

On Wed, 18 Nov 2009 13:58:17 -0600
Anthony Liguori <aliguori@linux.vnet.ibm.com> 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")
> >> - 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
> >>     
> >
> > 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.

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 18 Nov 2009 16:16:11 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> 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.

On 11/19/2009 09:42 AM, Markus Armbruster wrote:
>> >
>> >    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.

You're not sending QErrors on the wire yet, are you?  Once you do, I 
suspect it will be easiest to handle to_json for QError too (or make it 
a virtual method as it was in my patches...).

Paolo