From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwaJj-0005Jh-Nx for qemu-devel@nongnu.org; Thu, 25 Aug 2011 09:50:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwaJi-0002uf-Lx for qemu-devel@nongnu.org; Thu, 25 Aug 2011 09:49:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30695) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwaJi-0002uT-CK for qemu-devel@nongnu.org; Thu, 25 Aug 2011 09:49:58 -0400 Message-ID: <4E5653B4.9030206@redhat.com> Date: Thu, 25 Aug 2011 15:52:52 +0200 From: Kevin Wolf 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> <4E5650B8.9040809@codemonkey.ws> In-Reply-To: <4E5650B8.9040809@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-15 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: Anthony Liguori Cc: Luiz Capitulino , Michael Roth , qemu-devel@nongnu.org Am 25.08.2011 15:40, schrieb Anthony Liguori: > 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. I'm not saying that implementing a default value in generic (or generated) code works for all cases. But if the schema supported default values, we could get rid of the parameter in all simple cases (which I would expect to be the majority); and if there is no default value in the schema, we could still generate the has_* parameters. Kevin