From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37470 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PzDXg-0002i1-H5 for qemu-devel@nongnu.org; Mon, 14 Mar 2011 15:35:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PzDXf-00067j-0h for qemu-devel@nongnu.org; Mon, 14 Mar 2011 15:35:00 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:40889) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PzDXe-00067b-Sa for qemu-devel@nongnu.org; Mon, 14 Mar 2011 15:34:58 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e3.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p2EJEcET025149 for ; Mon, 14 Mar 2011 15:14:38 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 606F338C8038 for ; Mon, 14 Mar 2011 15:34:54 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p2EJYvVF382626 for ; Mon, 14 Mar 2011 15:34:57 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p2EJYv2P028910 for ; Mon, 14 Mar 2011 16:34:57 -0300 Message-ID: <4D7E6DDF.4070109@us.ibm.com> Date: Mon, 14 Mar 2011 14:34:55 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1299877249-13433-1-git-send-email-aliguori@us.ibm.com> <1299877249-13433-4-git-send-email-aliguori@us.ibm.com> <20110314161830.458ab6df@doriath> In-Reply-To: <20110314161830.458ab6df@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 03/11] add a generic Error object List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Paolo Bonzini , qemu-devel@nongnu.org, Michael D Roth , Markus Armbruster On 03/14/2011 02:18 PM, Luiz Capitulino wrote: > On Fri, 11 Mar 2011 15:00:41 -0600 > Anthony Liguori 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 >> >> 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 >> + * >> + * 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 >> + >> +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 >> + * >> + * 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