[Qemu-devel] Re: [PATCH 03/11] add a generic Error object

[Luiz Capitulino]

On Fri, 11 Mar 2011 15:00:41 -0600
Anthony Liguori <address@hidden> 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 <address@hidden>
> 
> 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   <address@hidden>
> + *
> + * 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   <address@hidden>
> + *
> + * 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   <address@hidden>
> + *
> + * 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