On Mon, 14 Mar 2011 14:34:55 -0500
Anthony Liguori <aligu...@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<aligu...@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<aligu...@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<aligu...@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<aligu...@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
>
