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

[Anthony Liguori]

On 03/14/2011 02:18 PM, Luiz Capitulino wrote:
> 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?

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

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

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

Right here.

Regards,

Anthony Liguori