From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:39222) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwaAH-0001sK-CK for qemu-devel@nongnu.org; Thu, 25 Aug 2011 09:40:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwaAG-0001Ks-1O for qemu-devel@nongnu.org; Thu, 25 Aug 2011 09:40:13 -0400 Received: from mail-gx0-f173.google.com ([209.85.161.173]:61556) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwaAF-0001Kl-UZ for qemu-devel@nongnu.org; Thu, 25 Aug 2011 09:40:11 -0400 Received: by gxk26 with SMTP id 26so1987243gxk.4 for ; Thu, 25 Aug 2011 06:40:11 -0700 (PDT) Message-ID: <4E5650B8.9040809@codemonkey.ws> Date: Thu, 25 Aug 2011 08:40:08 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1314211389-28915-1-git-send-email-aliguori@us.ibm.com> <1314211389-28915-9-git-send-email-aliguori@us.ibm.com> <4E563DD9.7030406@redhat.com> In-Reply-To: <4E563DD9.7030406@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Luiz Capitulino , Michael Roth , qemu-devel@nongnu.org On 08/25/2011 07:19 AM, Kevin Wolf wrote: > Am 24.08.2011 20:43, schrieb Anthony Liguori: >> Signed-off-by: Anthony Liguori >> --- >> blockdev.c | 22 +++++++++++----------- >> blockdev.h | 1 - >> hmp-commands.hx | 3 +-- >> hmp.c | 14 ++++++++++++++ >> hmp.h | 1 + >> qapi-schema.json | 25 +++++++++++++++++++++++++ >> qmp-commands.hx | 3 +-- >> 7 files changed, 53 insertions(+), 16 deletions(-) > > All of the conversion patches I've read so far add more lines than they > delete (even when you ignore changes to the schema, which is mostly new > documentation), even though I had expected code generation to do the > opposite, that is less hand-written code. > > Is this expected, or are these first examples just exceptions? Yes. These are extremely simple interfaces so unmarshalling a couple strings by hand really isn't all that hard to do. Plus, this series adds 4 new commands and also adds significantly more documentation than has ever existed before (in fact, that's the largest add in this patch). The real code savings comes in for the commands that return complex data structures like query-vnc. Not only do we save code, but we save a lot of complexity. In the full conversion branch, I think we're generating somewhere around 10k lines of code. So there's a pretty significant savings. > >> diff --git a/blockdev.c b/blockdev.c >> index d272659..6b7fc41 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -16,6 +16,7 @@ >> #include "sysemu.h" >> #include "hw/qdev.h" >> #include "block_int.h" >> +#include "qmp-commands.h" >> >> static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); >> >> @@ -644,32 +645,31 @@ out: >> return ret; >> } >> >> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force) >> +static int eject_device(BlockDriverState *bs, int force, Error **errp) >> { >> if (!bdrv_is_removable(bs)) { >> - qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >> + error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs)); >> return -1; >> } >> if (!force&& bdrv_is_locked(bs)) { >> - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >> + error_set(errp, QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); >> return -1; >> } >> bdrv_close(bs); >> return 0; >> } >> >> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data) >> +void qmp_eject(const char *device, bool has_force, bool force, Error **errp) > > Wow, this is ugly. :-) > > I would suspect that many cases of optional arguments are like this: If > it isn't specified, the very first thing the monitor handler does is to > assign a default value (false in this case). Can't we include default > values in the schema and get the handling outside instead of an > additional has_xyz parameter that can easily be ignored by accident, > like in the code below? There are quite a few commands that actually rely on tristate behavior. So they'll do things like: if (has_force) { if (force) { do_A(); } else { do_B(); } } else { do_C(); } It's not pretty, but it lets us preserve compatibility. I think it's also safer for dealing with pointers because otherwise you have a mix of pointers that may be null and may not be null. Having a clear indication of which pointers are nullable makes for safer code. Regards, Anthony Liguori > >> { >> BlockDriverState *bs; >> - int force = qdict_get_try_bool(qdict, "force", 0); >> - const char *filename = qdict_get_str(qdict, "device"); >> >> - bs = bdrv_find(filename); >> + bs = bdrv_find(device); >> if (!bs) { >> - qerror_report(QERR_DEVICE_NOT_FOUND, filename); >> - return -1; >> + error_set(errp, QERR_DEVICE_NOT_FOUND, device); >> + return; >> } >> - return eject_device(mon, bs, force); >> + >> + eject_device(bs, force, errp); >> } > > Kevin >