From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60309) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwYrU-0008C9-2P for qemu-devel@nongnu.org; Thu, 25 Aug 2011 08:16:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwYrS-0006xz-U9 for qemu-devel@nongnu.org; Thu, 25 Aug 2011 08:16:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61915) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwYrS-0006xi-My for qemu-devel@nongnu.org; Thu, 25 Aug 2011 08:16:42 -0400 Message-ID: <4E563DD9.7030406@redhat.com> Date: Thu, 25 Aug 2011 14:19:37 +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> In-Reply-To: <1314211389-28915-9-git-send-email-aliguori@us.ibm.com> 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: Michael Roth , qemu-devel@nongnu.org, Luiz Capitulino 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? > 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? > { > 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