From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43264) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuE4I-0005IA-OP for qemu-devel@nongnu.org; Wed, 25 Jul 2012 22:44:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuE4H-0003Ro-8w for qemu-devel@nongnu.org; Wed, 25 Jul 2012 22:44:50 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:49414) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuE4H-0003Ri-1l for qemu-devel@nongnu.org; Wed, 25 Jul 2012 22:44:49 -0400 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 25 Jul 2012 20:44:47 -0600 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 4A94619D803D for ; Thu, 26 Jul 2012 02:43:57 +0000 (WET) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q6Q2i0cZ156670 for ; Wed, 25 Jul 2012 20:44:00 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q6Q2i0ll015624 for ; Wed, 25 Jul 2012 20:44:00 -0600 From: Anthony Liguori In-Reply-To: <1343249431-9245-1-git-send-email-lcapitulino@redhat.com> References: <1343249431-9245-1-git-send-email-lcapitulino@redhat.com> Date: Wed, 25 Jul 2012 21:43:55 -0500 Message-ID: <87boj3dyxg.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [RFC 00/14]: add printf-like human msg to error_set() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, armbru@redhat.com, afaerber@suse.de, peter.maydell@linaro.org Luiz Capitulino writes: > Basically, this series changes a call like: > > error_set(errp, QERR_DEVICE_NOT_FOUND, device); > > to: > > error_set(errp, QERR_DEVICE_NOT_FOUND, > "Device 'device=%s' not found", device); > > In the first call, QERR_DEVICE_NOT_FOUND is a string containing a json dict: > > "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }" This is the wrong direction. Looking through the patch, this makes the code much more redundant overall. You have dozens of calls that are duplicating the same error message. This is not progress. We should just stick with a simple QERR_GENERIC and call it a day. Let's not needlessly complicate existing code. Regards, Anthony Liguori > > error_set() then uses that string and the 'device' argument to build the > error object, which is a QDict. The human error message is fixed, and exists > in the qerror_table[] array. That array is indexed, and the human error > message is added to the error object. > > In the new way, QERR_DEVICE_NOT_FOUND is an enumeration value and the > human error message is passed as an argument. qerror_table[] is gone. The > error object is built by using QERR_DEVICE_NOT_FOUND as an index for a table > containing all those json dict strings (this can, and probably will, be > eliminated, as the QMP code can generate its error object from the Error > object). > > An important detail is that, the error object data member is constructed > by parsing the human message string and looking for name=value pairs. If > a required data member (like 'device' above) is not found in the human message, > a default value is used ('unknown' for strings and 0 for integers). > > This series unlocks several possible simplification, like moving from: > > struct Error > { > QDict *obj; > const char *fmt; > char *msg; > }; > > to: > > struct Error > { > ErrClass err_class; > char *msg; > }; > > But I haven't done it all yet. > > Please, check individual patches for more details. > > balloon.c | 10 +- > block.c | 4 +- > block/cow.c | 3 +- > block/qcow.c | 7 +- > block/qcow2.c | 3 +- > block/qed.c | 3 +- > block/stream.c | 2 +- > block/vdi.c | 4 +- > block/vmdk.c | 4 +- > block/vpc.c | 4 +- > block/vvfat.c | 4 +- > blockdev.c | 79 +++++++------- > configure | 10 -- > cpus.c | 13 ++- > dump-stub.c | 2 +- > dump.c | 18 ++-- > error.c | 45 ++------ > error.h | 5 +- > hmp.c | 2 +- > hw/9pfs/virtio-9p.c | 3 +- > hw/ivshmem.c | 2 +- > hw/pci-stub.c | 2 +- > hw/pci.c | 4 +- > hw/qdev-addr.c | 6 +- > hw/qdev-monitor.c | 26 +++-- > hw/qdev-properties.c | 49 ++++----- > hw/qdev.c | 6 +- > hw/usb/bus.c | 3 +- > hw/usb/hcd-ehci.c | 6 +- > hw/usb/redirect.c | 5 +- > json-parser.c | 2 +- > migration.c | 6 +- > monitor.c | 127 +++++------------------ > net.c | 20 ++-- > qapi/qapi-visit-core.c | 22 ++-- > qapi/qmp-dispatch.c | 14 ++- > qapi/qmp-input-visitor.c | 22 ++-- > qapi/string-input-visitor.c | 12 +-- > qemu-config.c | 2 +- > qemu-ga.c | 4 +- > qemu-option.c | 20 ++-- > qemu-sockets.c | 22 ++-- > qerror.c | 243 ++++++++++++++------------------------------ > qerror.h | 22 +--- > qga/commands-posix.c | 78 +++++++------- > qga/commands-win32.c | 53 ++++------ > qmp.c | 47 +++++---- > qom/object.c | 20 ++-- > savevm.c | 6 +- > scripts/qapi-errors.py | 80 ++++++++------- > target-i386/cpu.c | 15 +-- > ui/vnc.c | 4 +- > 52 files changed, 450 insertions(+), 725 deletions(-)