From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1My3vO-0007dp-0X for qemu-devel@nongnu.org; Wed, 14 Oct 2009 09:29:54 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1My3vI-0007bn-7Z for qemu-devel@nongnu.org; Wed, 14 Oct 2009 09:29:52 -0400 Received: from [199.232.76.173] (port=32879 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1My3vI-0007bj-2y for qemu-devel@nongnu.org; Wed, 14 Oct 2009 09:29:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9921) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1My3vH-0000ji-JG for qemu-devel@nongnu.org; Wed, 14 Oct 2009 09:29:47 -0400 Message-ID: <4AD5D245.9050705@redhat.com> Date: Wed, 14 Oct 2009 15:29:41 +0200 From: Paolo Bonzini MIME-Version: 1.0 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> In-Reply-To: <877huy6hzm.fsf@pike.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Markus Armbruster Cc: aliguori@us.ibm.com, kraxel@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino 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." 3) I don't think this technique is used in qemu, but what about putting all errors in a header file like 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. Paolo