Patchwork [03/11] add a generic Error object

login
register
mail settings
Submitter Anthony Liguori
Date March 11, 2011, 9 p.m.
Message ID <1299877249-13433-4-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/86455/
State New
Headers show

Comments

Anthony Liguori - March 11, 2011, 9 p.m.
The Error class is similar to QError (now deprecated) except that it supports
propagation.  This allows for higher quality error handling.  It's losely
modeled after glib style GErrors.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Blue Swirl - March 12, 2011, 11:05 a.m.
On Fri, Mar 11, 2011 at 11:00 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> The Error class is similar to QError (now deprecated) except that it supports
> propagation.  This allows for higher quality error handling.  It's losely
> modeled after glib style GErrors.
>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 0ba02c7..da31530 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>  block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
> +block-obj-y += error.o
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>
> diff --git a/error.c b/error.c
> new file mode 100644
> index 0000000..5d84106
> --- /dev/null
> +++ b/error.c
> @@ -0,0 +1,122 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#include "error.h"
> +#include "error_int.h"
> +#include "qemu-objects.h"
> +#include "qerror.h"
> +#include <assert.h>
> +
> +struct Error
> +{
> +    QDict *obj;
> +    const char *fmt;
> +    char *msg;
> +};
> +
> +void error_set(Error **errp, const char *fmt, ...)
> +{
> +    Error *err;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +
> +    err = qemu_mallocz(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
> +    va_end(ap);
> +    err->fmt = fmt;
> +
> +    *errp = err;
> +}
> +
> +bool error_is_set(Error **errp)
> +{
> +    return (errp && *errp);
> +}
> +
> +const char *error_get_pretty(Error *err)
> +{
> +    if (err->msg == NULL) {
> +        QString *str;
> +        str = qerror_format(err->fmt, err->obj);
> +        err->msg = qemu_strdup(qstring_get_str(str));
> +    }
> +
> +    return err->msg;
> +}
> +
> +const char *error_get_field(Error *err, const char *field)
> +{
> +    if (strcmp(field, "class") == 0) {
> +        return qdict_get_str(err->obj, field);
> +    } else {
> +        QDict *dict = qdict_get_qdict(err->obj, "data");
> +        return qdict_get_str(dict, field);
> +    }
> +}
> +
> +void error_free(Error *err)
> +{
> +    QDECREF(err->obj);
> +    qemu_free(err->msg);
> +    qemu_free(err);
> +}
> +
> +bool error_is_type(Error *err, const char *fmt)
> +{
> +    char *ptr;
> +    char *end;
> +    char classname[1024];
> +
> +    ptr = strstr(fmt, "'class': '");
> +    assert(ptr != NULL);
> +    ptr += strlen("'class': '");
> +
> +    end = strchr(ptr, '\'');
> +    assert(end != NULL);
> +
> +    memcpy(classname, ptr, (end - ptr));
> +    classname[(end - ptr)] = 0;
> +
> +    return strcmp(classname, error_get_field(err, "class")) == 0;
> +}
> +
> +void error_propagate(Error **dst_err, Error *local_err)
> +{
> +    if (dst_err) {
> +        *dst_err = local_err;
> +    } else if (local_err) {
> +        error_free(local_err);
> +    }
> +}
> +
> +QObject *error_get_qobject(Error *err)
> +{
> +    QINCREF(err->obj);
> +    return QOBJECT(err->obj);
> +}
> +
> +void error_set_qobject(Error **errp, QObject *obj)
> +{
> +    Error *err;
> +    if (errp == NULL) {
> +        return;
> +    }
> +    err = qemu_mallocz(sizeof(*err));
> +    err->obj = qobject_to_qdict(obj);
> +    qobject_incref(obj);
> +
> +    *errp = err;
> +}
> diff --git a/error.h b/error.h
> new file mode 100644

The name is too generic, it could conflict with system headers. At
least I have /usr/include/error.h.

> index 0000000..317d487
> --- /dev/null
> +++ b/error.h
> @@ -0,0 +1,65 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef ERROR_H
> +#define ERROR_H

This #define could also conflict with system headers. In my system,
_ERROR_H is used, but who knows all error.h files out there?
Anthony Liguori - March 12, 2011, 2:51 p.m.
On 03/12/2011 05:05 AM, Blue Swirl wrote:
> On Fri, Mar 11, 2011 at 11:00 PM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>> The Error class is similar to QError (now deprecated) except that it supports
>> propagation.  This allows for higher quality error handling.  It's losely
>> modeled after glib style GErrors.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 0ba02c7..da31530 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>>
>>   block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>>   block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
>> +block-obj-y += error.o
>>   block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>
>> diff --git a/error.c b/error.c
>> new file mode 100644
>> index 0000000..5d84106
>> --- /dev/null
>> +++ b/error.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#include "error.h"
>> +#include "error_int.h"
>> +#include "qemu-objects.h"
>> +#include "qerror.h"
>> +#include<assert.h>
>> +
>> +struct Error
>> +{
>> +    QDict *obj;
>> +    const char *fmt;
>> +    char *msg;
>> +};
>> +
>> +void error_set(Error **errp, const char *fmt, ...)
>> +{
>> +    Error *err;
>> +    va_list ap;
>> +
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +
>> +    err = qemu_mallocz(sizeof(*err));
>> +
>> +    va_start(ap, fmt);
>> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt,&ap));
>> +    va_end(ap);
>> +    err->fmt = fmt;
>> +
>> +    *errp = err;
>> +}
>> +
>> +bool error_is_set(Error **errp)
>> +{
>> +    return (errp&&  *errp);
>> +}
>> +
>> +const char *error_get_pretty(Error *err)
>> +{
>> +    if (err->msg == NULL) {
>> +        QString *str;
>> +        str = qerror_format(err->fmt, err->obj);
>> +        err->msg = qemu_strdup(qstring_get_str(str));
>> +    }
>> +
>> +    return err->msg;
>> +}
>> +
>> +const char *error_get_field(Error *err, const char *field)
>> +{
>> +    if (strcmp(field, "class") == 0) {
>> +        return qdict_get_str(err->obj, field);
>> +    } else {
>> +        QDict *dict = qdict_get_qdict(err->obj, "data");
>> +        return qdict_get_str(dict, field);
>> +    }
>> +}
>> +
>> +void error_free(Error *err)
>> +{
>> +    QDECREF(err->obj);
>> +    qemu_free(err->msg);
>> +    qemu_free(err);
>> +}
>> +
>> +bool error_is_type(Error *err, const char *fmt)
>> +{
>> +    char *ptr;
>> +    char *end;
>> +    char classname[1024];
>> +
>> +    ptr = strstr(fmt, "'class': '");
>> +    assert(ptr != NULL);
>> +    ptr += strlen("'class': '");
>> +
>> +    end = strchr(ptr, '\'');
>> +    assert(end != NULL);
>> +
>> +    memcpy(classname, ptr, (end - ptr));
>> +    classname[(end - ptr)] = 0;
>> +
>> +    return strcmp(classname, error_get_field(err, "class")) == 0;
>> +}
>> +
>> +void error_propagate(Error **dst_err, Error *local_err)
>> +{
>> +    if (dst_err) {
>> +        *dst_err = local_err;
>> +    } else if (local_err) {
>> +        error_free(local_err);
>> +    }
>> +}
>> +
>> +QObject *error_get_qobject(Error *err)
>> +{
>> +    QINCREF(err->obj);
>> +    return QOBJECT(err->obj);
>> +}
>> +
>> +void error_set_qobject(Error **errp, QObject *obj)
>> +{
>> +    Error *err;
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +    err = qemu_mallocz(sizeof(*err));
>> +    err->obj = qobject_to_qdict(obj);
>> +    qobject_incref(obj);
>> +
>> +    *errp = err;
>> +}
>> diff --git a/error.h b/error.h
>> new file mode 100644
> The name is too generic, it could conflict with system headers. At
> least I have /usr/include/error.h.
>
>> index 0000000..317d487
>> --- /dev/null
>> +++ b/error.h
>> @@ -0,0 +1,65 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#ifndef ERROR_H
>> +#define ERROR_H
> This #define could also conflict with system headers. In my system,
> _ERROR_H is used, but who knows all error.h files out there?

Certainly a good point.  I'll use a different name.

Regards,

Anthony Liguori
Luiz Capitulino - March 14, 2011, 7:18 p.m.
On Fri, 11 Mar 2011 15:00:41 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> The Error class is similar to QError (now deprecated) except that it supports
> propagation.  This allows for higher quality error handling.  It's losely
> modeled after glib style GErrors.

I think Daniel asked this, but I can't remember your answer: why don't we
use GError then?

Also, I think this patch needs more description regarding how this is going
to replace QError. I mean, we want to deprecate QError but it seems to me
that you're going to maintain the error declaration format, like:

 "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"

And the current QError class list in qerror.h. Did I get it right?

More comments below.

> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 0ba02c7..da31530 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>  
>  block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>  block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
> +block-obj-y += error.o
>  block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>  block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o

You also have to do this:

-CHECK_PROG_DEPS = qemu-malloc.o $(oslib-obj-y) $(trace-obj-y)
+CHECK_PROG_DEPS = qemu-malloc.o error.o qerror.o $(oslib-obj-y) $(trace-obj-y)

Otherwise you break check build.

> diff --git a/error.c b/error.c
> new file mode 100644
> index 0000000..5d84106
> --- /dev/null
> +++ b/error.c
> @@ -0,0 +1,122 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#include "error.h"
> +#include "error_int.h"
> +#include "qemu-objects.h"
> +#include "qerror.h"
> +#include <assert.h>
> +
> +struct Error
> +{
> +    QDict *obj;

'obj' is a bit generic and sometimes it's used to denote QObjects in the
code, I suggest 'error_dict' or something that communicates its purpose.

> +    const char *fmt;
> +    char *msg;

I don't think we should store 'msg', more about this in error_get_pretty().

> +};
> +
> +void error_set(Error **errp, const char *fmt, ...)
> +{
> +    Error *err;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +
> +    err = qemu_mallocz(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
> +    va_end(ap);
> +    err->fmt = fmt;
> +
> +    *errp = err;
> +}
> +
> +bool error_is_set(Error **errp)
> +{
> +    return (errp && *errp);
> +}
> +
> +const char *error_get_pretty(Error *err)
> +{
> +    if (err->msg == NULL) {
> +        QString *str;
> +        str = qerror_format(err->fmt, err->obj);
> +        err->msg = qemu_strdup(qstring_get_str(str));

Four comments here:

 1. This is missing a QDECREF(str);

 2. Storing 'msg' looks like an unnecessary optimization to me, why don't
    we just re-create it when error_get_pretty() is called?

 3. This function is not used by this series

 4. I think it's a good idea to assert on Error == NULL, specially
    because some functions accept it

> +    }
> +
> +    return err->msg;
> +}
> +
> +const char *error_get_field(Error *err, const char *field)
> +{
> +    if (strcmp(field, "class") == 0) {
> +        return qdict_get_str(err->obj, field);
> +    } else {
> +        QDict *dict = qdict_get_qdict(err->obj, "data");
> +        return qdict_get_str(dict, field);
> +    }
> +}
> +
> +void error_free(Error *err)
> +{
> +    QDECREF(err->obj);
> +    qemu_free(err->msg);
> +    qemu_free(err);
> +}
> +
> +bool error_is_type(Error *err, const char *fmt)
> +{
> +    char *ptr;
> +    char *end;
> +    char classname[1024];
> +
> +    ptr = strstr(fmt, "'class': '");
> +    assert(ptr != NULL);
> +    ptr += strlen("'class': '");
> +
> +    end = strchr(ptr, '\'');
> +    assert(end != NULL);
> +    
> +    memcpy(classname, ptr, (end - ptr));
> +    classname[(end - ptr)] = 0;
> +
> +    return strcmp(classname, error_get_field(err, "class")) == 0;
> +}

Not used by this series. Except for obvious stuff, I prefer to only add
code that's going to be used right away.

> +
> +void error_propagate(Error **dst_err, Error *local_err)
> +{
> +    if (dst_err) {
> +        *dst_err = local_err;
> +    } else if (local_err) {
> +        error_free(local_err);
> +    }
> +}
> +
> +QObject *error_get_qobject(Error *err)
> +{
> +    QINCREF(err->obj);
> +    return QOBJECT(err->obj);
> +}
> +
> +void error_set_qobject(Error **errp, QObject *obj)
> +{
> +    Error *err;
> +    if (errp == NULL) {
> +        return;
> +    }
> +    err = qemu_mallocz(sizeof(*err));
> +    err->obj = qobject_to_qdict(obj);
> +    qobject_incref(obj);
> +
> +    *errp = err;
> +}

This is not documented. Also, I prefer the documentation & code next to each
other in the same file.

> diff --git a/error.h b/error.h
> new file mode 100644
> index 0000000..317d487
> --- /dev/null
> +++ b/error.h
> @@ -0,0 +1,65 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef ERROR_H
> +#define ERROR_H
> +
> +#include <stdbool.h>
> +
> +/**
> + * A class representing internal errors within QEMU.  An error has a string
> + * typename and optionally a set of named string parameters.
> + */
> +typedef struct Error Error;
> +
> +/**
> + * Set an indirect pointer to an error given a printf-style format parameter.
> + * Currently, qerror.h defines these error formats.  This function is not
> + * meant to be used outside of QEMU.
> + */
> +void error_set(Error **err, const char *fmt, ...)
> +    __attribute__((format(printf, 2, 3)));
> +
> +/**
> + * Returns true if an indirect pointer to an error is pointing to a valid
> + * error object.
> + */
> +bool error_is_set(Error **err);
> +
> +/**
> + * Get a human readable representation of an error object.
> + */
> +const char *error_get_pretty(Error *err);
> +
> +/**
> + * Get an individual named error field.
> + */
> +const char *error_get_field(Error *err, const char *field);
> +
> +/**
> + * Propagate an error to an indirect pointer to an error.  This function will
> + * always transfer ownership of the error reference and handles the case where
> + * dst_err is NULL correctly.
> + */
> +void error_propagate(Error **dst_err, Error *local_err);
> +
> +/**
> + * Free an error object.
> + */
> +void error_free(Error *err);
> +
> +/**
> + * Determine if an error is of a speific type (based on the qerror format).
> + * Non-QEMU users should get the `class' field to identify the error type.
> + */
> +bool error_is_type(Error *err, const char *fmt);
> +
> +#endif
> diff --git a/error_int.h b/error_int.h
> new file mode 100644
> index 0000000..eaba65e
> --- /dev/null
> +++ b/error_int.h
> @@ -0,0 +1,27 @@
> +/*
> + * QEMU Error Objects
> + *
> + * Copyright IBM, Corp. 2011
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> + * the COPYING.LIB file in the top-level directory.
> + */
> +#ifndef QEMU_ERROR_INT_H
> +#define QEMU_ERROR_INT_H
> +
> +#include "qemu-common.h"
> +#include "qobject.h"
> +#include "error.h"
> +
> +/**
> + * Internal QEMU functions for working with Error.
> + *
> + * These are used to convert QErrors to Errors
> + */
> +QObject *error_get_qobject(Error *err);
> +void error_set_qobject(Error **errp, QObject *obj);
> +  
> +#endif
Anthony Liguori - March 14, 2011, 7:34 p.m.
On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> On Fri, 11 Mar 2011 15:00:41 -0600
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> The Error class is similar to QError (now deprecated) except that it supports
>> propagation.  This allows for higher quality error handling.  It's losely
>> modeled after glib style GErrors.
> I think Daniel asked this, but I can't remember your answer: why don't we
> use GError then?

Because GError just uses strings and doesn't store key/values.

> Also, I think this patch needs more description regarding how this is going
> to replace QError.

Once there is no more qerror usage (we need to converting remaining HMP 
commands to QMP), qerror goes away.  This is scoped for the 0.15 release.

>   I mean, we want to deprecate QError but it seems to me
> that you're going to maintain the error declaration format, like:
>
>   "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
>
> And the current QError class list in qerror.h. Did I get it right?

No, it will be switched to something simpler.  The QERR JSON is just an 
implementation detail.

> More comments below.
>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 0ba02c7..da31530 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
>>
>>   block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
>>   block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
>> +block-obj-y += error.o
>>   block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
>>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> You also have to do this:
>
> -CHECK_PROG_DEPS = qemu-malloc.o $(oslib-obj-y) $(trace-obj-y)
> +CHECK_PROG_DEPS = qemu-malloc.o error.o qerror.o $(oslib-obj-y) $(trace-obj-y)
>
> Otherwise you break check build.

Ah, okay.  Maybe I'll convert those over to gtester while I'm there.

>> diff --git a/error.c b/error.c
>> new file mode 100644
>> index 0000000..5d84106
>> --- /dev/null
>> +++ b/error.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#include "error.h"
>> +#include "error_int.h"
>> +#include "qemu-objects.h"
>> +#include "qerror.h"
>> +#include<assert.h>
>> +
>> +struct Error
>> +{
>> +    QDict *obj;
> 'obj' is a bit generic and sometimes it's used to denote QObjects in the
> code, I suggest 'error_dict' or something that communicates its purpose.

Sure.

>> +const char *error_get_pretty(Error *err)
>> +{
>> +    if (err->msg == NULL) {
>> +        QString *str;
>> +        str = qerror_format(err->fmt, err->obj);
>> +        err->msg = qemu_strdup(qstring_get_str(str));
> Four comments here:
>
>   1. This is missing a QDECREF(str);

Yes, I caught this a few days ago in my tree thanks to valgrind.

>   2. Storing 'msg' looks like an unnecessary optimization to me, why don't
>      we just re-create it when error_get_pretty() is called?

Because we return a 'const char *' here so by storing msg, we can tie 
the string's life cycle to the Error object.  That means callers don't 
have to worry about freeing it.

>   3. This function is not used by this series

Yeah, it's infrastructure that needs to be here for subsequent series.

>   4. I think it's a good idea to assert on Error == NULL, specially
>      because some functions accept it

Only functions that take a double pointer, but not a bad thing to do.

>> +bool error_is_type(Error *err, const char *fmt)
>> +{
>> +    char *ptr;
>> +    char *end;
>> +    char classname[1024];
>> +
>> +    ptr = strstr(fmt, "'class': '");
>> +    assert(ptr != NULL);
>> +    ptr += strlen("'class': '");
>> +
>> +    end = strchr(ptr, '\'');
>> +    assert(end != NULL);
>> +
>> +    memcpy(classname, ptr, (end - ptr));
>> +    classname[(end - ptr)] = 0;
>> +
>> +    return strcmp(classname, error_get_field(err, "class")) == 0;
>> +}
> Not used by this series. Except for obvious stuff, I prefer to only add
> code that's going to be used right away.

That means adding a ton of stuff all at once.  Splitting it up like this 
is pretty reasonable IMHO.   Think of Error as an interface and not just 
a code dependency.

>> +
>> +void error_propagate(Error **dst_err, Error *local_err)
>> +{
>> +    if (dst_err) {
>> +        *dst_err = local_err;
>> +    } else if (local_err) {
>> +        error_free(local_err);
>> +    }
>> +}
>> +
>> +QObject *error_get_qobject(Error *err)
>> +{
>> +    QINCREF(err->obj);
>> +    return QOBJECT(err->obj);
>> +}
>> +
>> +void error_set_qobject(Error **errp, QObject *obj)
>> +{
>> +    Error *err;
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +    err = qemu_mallocz(sizeof(*err));
>> +    err->obj = qobject_to_qdict(obj);
>> +    qobject_incref(obj);
>> +
>> +    *errp = err;
>> +}
> This is not documented. Also, I prefer the documentation&  code next to each
> other in the same file.

These are internal functions to QEMU and are documented as such.

>> diff --git a/error_int.h b/error_int.h
>> new file mode 100644
>> index 0000000..eaba65e
>> --- /dev/null
>> +++ b/error_int.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * QEMU Error Objects
>> + *
>> + * Copyright IBM, Corp. 2011
>> + *
>> + * Authors:
>> + *  Anthony Liguori<aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
>> + * the COPYING.LIB file in the top-level directory.
>> + */
>> +#ifndef QEMU_ERROR_INT_H
>> +#define QEMU_ERROR_INT_H
>> +
>> +#include "qemu-common.h"
>> +#include "qobject.h"
>> +#include "error.h"
>> +
>> +/**
>> + * Internal QEMU functions for working with Error.
>> + *
>> + * These are used to convert QErrors to Errors
>> + */
>> +QObject *error_get_qobject(Error *err);
>> +void error_set_qobject(Error **errp, QObject *obj);
>> +
>> +#endif

Right here.

Regards,

Anthony Liguori
Luiz Capitulino - March 14, 2011, 7:57 p.m.
On Mon, 14 Mar 2011 14:34:55 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> > On Fri, 11 Mar 2011 15:00:41 -0600
> > Anthony Liguori<aliguori@us.ibm.com>  wrote:
> >
> >> The Error class is similar to QError (now deprecated) except that it supports
> >> propagation.  This allows for higher quality error handling.  It's losely
> >> modeled after glib style GErrors.
> > I think Daniel asked this, but I can't remember your answer: why don't we
> > use GError then?
> 
> Because GError just uses strings and doesn't store key/values.
> 
> > Also, I think this patch needs more description regarding how this is going
> > to replace QError.
> 
> Once there is no more qerror usage (we need to converting remaining HMP 
> commands to QMP), qerror goes away.  This is scoped for the 0.15 release.
> 
> >   I mean, we want to deprecate QError but it seems to me
> > that you're going to maintain the error declaration format, like:
> >
> >   "{ 'class': 'CommandNotFound', 'data': { 'name': %s } }"
> >
> > And the current QError class list in qerror.h. Did I get it right?
> 
> No, it will be switched to something simpler.  The QERR JSON is just an 
> implementation detail.
> 
> > More comments below.
> >
> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
> >>
> >> diff --git a/Makefile.objs b/Makefile.objs
> >> index 0ba02c7..da31530 100644
> >> --- a/Makefile.objs
> >> +++ b/Makefile.objs
> >> @@ -15,6 +15,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
> >>
> >>   block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
> >>   block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
> >> +block-obj-y += error.o
> >>   block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
> >>   block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> > You also have to do this:
> >
> > -CHECK_PROG_DEPS = qemu-malloc.o $(oslib-obj-y) $(trace-obj-y)
> > +CHECK_PROG_DEPS = qemu-malloc.o error.o qerror.o $(oslib-obj-y) $(trace-obj-y)
> >
> > Otherwise you break check build.
> 
> Ah, okay.  Maybe I'll convert those over to gtester while I'm there.
> 
> >> diff --git a/error.c b/error.c
> >> new file mode 100644
> >> index 0000000..5d84106
> >> --- /dev/null
> >> +++ b/error.c
> >> @@ -0,0 +1,122 @@
> >> +/*
> >> + * QEMU Error Objects
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> >> + * the COPYING.LIB file in the top-level directory.
> >> + */
> >> +#include "error.h"
> >> +#include "error_int.h"
> >> +#include "qemu-objects.h"
> >> +#include "qerror.h"
> >> +#include<assert.h>
> >> +
> >> +struct Error
> >> +{
> >> +    QDict *obj;
> > 'obj' is a bit generic and sometimes it's used to denote QObjects in the
> > code, I suggest 'error_dict' or something that communicates its purpose.
> 
> Sure.
> 
> >> +const char *error_get_pretty(Error *err)
> >> +{
> >> +    if (err->msg == NULL) {
> >> +        QString *str;
> >> +        str = qerror_format(err->fmt, err->obj);
> >> +        err->msg = qemu_strdup(qstring_get_str(str));
> > Four comments here:
> >
> >   1. This is missing a QDECREF(str);
> 
> Yes, I caught this a few days ago in my tree thanks to valgrind.
> 
> >   2. Storing 'msg' looks like an unnecessary optimization to me, why don't
> >      we just re-create it when error_get_pretty() is called?
> 
> Because we return a 'const char *' here so by storing msg, we can tie 
> the string's life cycle to the Error object.  That means callers don't 
> have to worry about freeing it.

Makes sense.

> 
> >   3. This function is not used by this series
> 
> Yeah, it's infrastructure that needs to be here for subsequent series.
> 
> >   4. I think it's a good idea to assert on Error == NULL, specially
> >      because some functions accept it
> 
> Only functions that take a double pointer, but not a bad thing to do.
> 
> >> +bool error_is_type(Error *err, const char *fmt)
> >> +{
> >> +    char *ptr;
> >> +    char *end;
> >> +    char classname[1024];
> >> +
> >> +    ptr = strstr(fmt, "'class': '");
> >> +    assert(ptr != NULL);
> >> +    ptr += strlen("'class': '");
> >> +
> >> +    end = strchr(ptr, '\'');
> >> +    assert(end != NULL);
> >> +
> >> +    memcpy(classname, ptr, (end - ptr));
> >> +    classname[(end - ptr)] = 0;
> >> +
> >> +    return strcmp(classname, error_get_field(err, "class")) == 0;
> >> +}
> > Not used by this series. Except for obvious stuff, I prefer to only add
> > code that's going to be used right away.
> 
> That means adding a ton of stuff all at once.  Splitting it up like this 
> is pretty reasonable IMHO.   Think of Error as an interface and not just 
> a code dependency.

But it's harder to review and to test.

> 
> >> +
> >> +void error_propagate(Error **dst_err, Error *local_err)
> >> +{
> >> +    if (dst_err) {
> >> +        *dst_err = local_err;
> >> +    } else if (local_err) {
> >> +        error_free(local_err);
> >> +    }
> >> +}
> >> +
> >> +QObject *error_get_qobject(Error *err)
> >> +{
> >> +    QINCREF(err->obj);
> >> +    return QOBJECT(err->obj);
> >> +}
> >> +
> >> +void error_set_qobject(Error **errp, QObject *obj)
> >> +{
> >> +    Error *err;
> >> +    if (errp == NULL) {
> >> +        return;
> >> +    }
> >> +    err = qemu_mallocz(sizeof(*err));
> >> +    err->obj = qobject_to_qdict(obj);
> >> +    qobject_incref(obj);
> >> +
> >> +    *errp = err;
> >> +}
> > This is not documented. Also, I prefer the documentation&  code next to each
> > other in the same file.
> 
> These are internal functions to QEMU and are documented as such.
> 
> >> diff --git a/error_int.h b/error_int.h
> >> new file mode 100644
> >> index 0000000..eaba65e
> >> --- /dev/null
> >> +++ b/error_int.h
> >> @@ -0,0 +1,27 @@
> >> +/*
> >> + * QEMU Error Objects
> >> + *
> >> + * Copyright IBM, Corp. 2011
> >> + *
> >> + * Authors:
> >> + *  Anthony Liguori<aliguori@us.ibm.com>
> >> + *
> >> + * This work is licensed under the terms of the GNU LGPL, version 2.  See
> >> + * the COPYING.LIB file in the top-level directory.
> >> + */
> >> +#ifndef QEMU_ERROR_INT_H
> >> +#define QEMU_ERROR_INT_H
> >> +
> >> +#include "qemu-common.h"
> >> +#include "qobject.h"
> >> +#include "error.h"
> >> +
> >> +/**
> >> + * Internal QEMU functions for working with Error.
> >> + *
> >> + * These are used to convert QErrors to Errors
> >> + */
> >> +QObject *error_get_qobject(Error *err);
> >> +void error_set_qobject(Error **errp, QObject *obj);
> >> +
> >> +#endif
> 
> Right here.
> 
> Regards,
> 
> Anthony Liguori
>

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 0ba02c7..da31530 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -15,6 +15,7 @@  oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
 block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
+block-obj-y += error.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
diff --git a/error.c b/error.c
new file mode 100644
index 0000000..5d84106
--- /dev/null
+++ b/error.c
@@ -0,0 +1,122 @@ 
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#include "error.h"
+#include "error_int.h"
+#include "qemu-objects.h"
+#include "qerror.h"
+#include <assert.h>
+
+struct Error
+{
+    QDict *obj;
+    const char *fmt;
+    char *msg;
+};
+
+void error_set(Error **errp, const char *fmt, ...)
+{
+    Error *err;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+
+    err = qemu_mallocz(sizeof(*err));
+
+    va_start(ap, fmt);
+    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
+    va_end(ap);
+    err->fmt = fmt;
+
+    *errp = err;
+}
+
+bool error_is_set(Error **errp)
+{
+    return (errp && *errp);
+}
+
+const char *error_get_pretty(Error *err)
+{
+    if (err->msg == NULL) {
+        QString *str;
+        str = qerror_format(err->fmt, err->obj);
+        err->msg = qemu_strdup(qstring_get_str(str));
+    }
+
+    return err->msg;
+}
+
+const char *error_get_field(Error *err, const char *field)
+{
+    if (strcmp(field, "class") == 0) {
+        return qdict_get_str(err->obj, field);
+    } else {
+        QDict *dict = qdict_get_qdict(err->obj, "data");
+        return qdict_get_str(dict, field);
+    }
+}
+
+void error_free(Error *err)
+{
+    QDECREF(err->obj);
+    qemu_free(err->msg);
+    qemu_free(err);
+}
+
+bool error_is_type(Error *err, const char *fmt)
+{
+    char *ptr;
+    char *end;
+    char classname[1024];
+
+    ptr = strstr(fmt, "'class': '");
+    assert(ptr != NULL);
+    ptr += strlen("'class': '");
+
+    end = strchr(ptr, '\'');
+    assert(end != NULL);
+    
+    memcpy(classname, ptr, (end - ptr));
+    classname[(end - ptr)] = 0;
+
+    return strcmp(classname, error_get_field(err, "class")) == 0;
+}
+
+void error_propagate(Error **dst_err, Error *local_err)
+{
+    if (dst_err) {
+        *dst_err = local_err;
+    } else if (local_err) {
+        error_free(local_err);
+    }
+}
+
+QObject *error_get_qobject(Error *err)
+{
+    QINCREF(err->obj);
+    return QOBJECT(err->obj);
+}
+
+void error_set_qobject(Error **errp, QObject *obj)
+{
+    Error *err;
+    if (errp == NULL) {
+        return;
+    }
+    err = qemu_mallocz(sizeof(*err));
+    err->obj = qobject_to_qdict(obj);
+    qobject_incref(obj);
+
+    *errp = err;
+}
diff --git a/error.h b/error.h
new file mode 100644
index 0000000..317d487
--- /dev/null
+++ b/error.h
@@ -0,0 +1,65 @@ 
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#ifndef ERROR_H
+#define ERROR_H
+
+#include <stdbool.h>
+
+/**
+ * A class representing internal errors within QEMU.  An error has a string
+ * typename and optionally a set of named string parameters.
+ */
+typedef struct Error Error;
+
+/**
+ * Set an indirect pointer to an error given a printf-style format parameter.
+ * Currently, qerror.h defines these error formats.  This function is not
+ * meant to be used outside of QEMU.
+ */
+void error_set(Error **err, const char *fmt, ...)
+    __attribute__((format(printf, 2, 3)));
+
+/**
+ * Returns true if an indirect pointer to an error is pointing to a valid
+ * error object.
+ */
+bool error_is_set(Error **err);
+
+/**
+ * Get a human readable representation of an error object.
+ */
+const char *error_get_pretty(Error *err);
+
+/**
+ * Get an individual named error field.
+ */
+const char *error_get_field(Error *err, const char *field);
+
+/**
+ * Propagate an error to an indirect pointer to an error.  This function will
+ * always transfer ownership of the error reference and handles the case where
+ * dst_err is NULL correctly.
+ */
+void error_propagate(Error **dst_err, Error *local_err);
+
+/**
+ * Free an error object.
+ */
+void error_free(Error *err);
+
+/**
+ * Determine if an error is of a speific type (based on the qerror format).
+ * Non-QEMU users should get the `class' field to identify the error type.
+ */
+bool error_is_type(Error *err, const char *fmt);
+
+#endif
diff --git a/error_int.h b/error_int.h
new file mode 100644
index 0000000..eaba65e
--- /dev/null
+++ b/error_int.h
@@ -0,0 +1,27 @@ 
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#ifndef QEMU_ERROR_INT_H
+#define QEMU_ERROR_INT_H
+
+#include "qemu-common.h"
+#include "qobject.h"
+#include "error.h"
+
+/**
+ * Internal QEMU functions for working with Error.
+ *
+ * These are used to convert QErrors to Errors
+ */
+QObject *error_get_qobject(Error *err);
+void error_set_qobject(Error **errp, QObject *obj);
+  
+#endif