[03/11] add a generic Error object

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>

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?

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

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

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

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
>