From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1My6w7-0003C6-GX for qemu-devel@nongnu.org; Wed, 14 Oct 2009 12:42:51 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1My6w2-00035k-Jh for qemu-devel@nongnu.org; Wed, 14 Oct 2009 12:42:50 -0400 Received: from [199.232.76.173] (port=60710 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1My6w2-00035J-CX for qemu-devel@nongnu.org; Wed, 14 Oct 2009 12:42:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14019) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1My6w1-0008P7-SH for qemu-devel@nongnu.org; Wed, 14 Oct 2009 12:42:46 -0400 Date: Wed, 14 Oct 2009 13:42:35 -0300 From: Luiz Capitulino Message-ID: <20091014134235.450f7b56@doriath> In-Reply-To: <4AD5D245.9050705@redhat.com> References: <1255453026-18637-1-git-send-email-lcapitulino@redhat.com> <1255453026-18637-8-git-send-email-lcapitulino@redhat.com> <877huy6hzm.fsf@pike.pond.sub.org> <4AD5D245.9050705@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 7/9] qdev: Use QError for not found error List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, kraxel@redhat.com, Markus Armbruster , qemu-devel@nongnu.org On Wed, 14 Oct 2009 15:29:41 +0200 Paolo Bonzini wrote: > On 10/14/2009 12:34 AM, Markus Armbruster wrote: > > Luiz Capitulino writes: > > > >> Signed-off-by: Luiz Capitulino > >> --- > >> hw/qdev.c | 4 ++-- > >> 1 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/qdev.c b/hw/qdev.c > >> index 906e897..3ce48f7 100644 > >> --- a/hw/qdev.c > >> +++ b/hw/qdev.c > >> @@ -28,6 +28,7 @@ > >> #include "net.h" > >> #include "qdev.h" > >> #include "sysemu.h" > >> +#include "qerror.h" > >> #include "monitor.h" > >> > >> static int qdev_hotplug = 0; > >> @@ -176,8 +177,7 @@ DeviceState *qdev_device_add(QemuOpts *opts) > >> /* find driver */ > >> info = qdev_find_info(NULL, driver); > >> if (!info) { > >> - qemu_error("Device \"%s\" not found. Try -device '?' for a list.\n", > >> - driver); > >> + qemu_error_structed(QERR_QDEV_NFOUND, "{ s: s }", "name", driver); > >> return NULL; > >> } > >> if (info->no_user) { > > > > And here we pay the price for structured error objects: reporting an > > error becomes more difficult. The harder you make reporting errors, the > > fewer errors get caught and reported. Also, the result is harder to > > read. > > If you count also the new code in 6/9, reporting an error definitely > becomes harder. Finding a middle ground would be great, and I think > that the right solution is to put as many things as possible in a single > place. For example: > > 1) there is a lot of code that is duplicated in every occurrence of an > error. One possibility to avoid this, would be to use a more > printf-like syntax for qemu_object_from_va, for example one of these: > > .... "{ name: %s }", driver); > .... "{ 'name': %s }", driver); > > Then you can move the first argument to the error table: > > + .code = QERR_QDEV_NFOUND, > + .desc = "device not found", > + .keys = "{ name: %s }" > > so that the qemu_error_structed call looks like > > qemu_error_structed (QERR_QDEV_NFOUND, driver); > > 2) do we need full flexibility of a function for printing errors? What > about adding a print-from-qobject function, so that you could replace > qerror_table[].user_print with a string like > > .formatted_print = "Device \"%{name}\" not found. Try -device" > " '?' for a list." I like both, but I see them as future improvements. > 3) I don't think this technique is used in qemu, but what about putting > all errors in a header file like We have qemu-monitor.hx, which is something like it. > QEMU_ERROR(QERR_DEV_NFOUND, > "device not found", > whatever); > > Then you can use the header file to generate both the enum and the error > table: > > typedef enum QErrorCode { > #define QEMU_ERROR(code_, ...) code_, > #include "qemu.def" > #undef QEMU_ERROR > } QErrorCode; > > ... > > static QErrorTable qerror_table[] = { > #define QEMU_ERROR(code_, desc_, data_) \ > { .code = (code_), .desc = (desc_), .data = (data_), > #include "qemu-error.def" > #undef QEMU_ERROR > }; > > This has the disadvantage of not allowing usage of designated initializers. Not sure if it really helps.