From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35784) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzWF4-0007Yr-Dc for qemu-devel@nongnu.org; Fri, 02 Sep 2011 12:05:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzWF3-0006Ya-7w for qemu-devel@nongnu.org; Fri, 02 Sep 2011 12:05:18 -0400 Received: from mail-gx0-f173.google.com ([209.85.161.173]:43061) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzWF3-0006YU-3q for qemu-devel@nongnu.org; Fri, 02 Sep 2011 12:05:17 -0400 Received: by gxk26 with SMTP id 26so2062750gxk.4 for ; Fri, 02 Sep 2011 09:05:16 -0700 (PDT) Message-ID: <4E60FEB9.8060508@codemonkey.ws> Date: Fri, 02 Sep 2011 11:05:13 -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> <4E5650B8.9040809@codemonkey.ws> <4E5653B4.9030206@redhat.com> In-Reply-To: <4E5653B4.9030206@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: qemu-devel@nongnu.org, Michael Roth , Luiz Capitulino On 08/25/2011 08:52 AM, Kevin Wolf wrote: > 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. I had thought about this but forgot to respond. The problem is that the schema doesn't help the C API. We're going to use QMP commands internally too. So if the schema defaulted optional arguments, you'd end up having those default values open coded in the C APIs. As ugly as it is, having an explicit not-specified flag allows the C API to have the same semantics as the wire protocol. Honestly, having optional arguments in the methods was a bad move. We shouldn't do that anymore. Optional arguments should always be done via a structure as Avi sort of suggested in another response. Regards, Anthony Liguori > > Kevin >