[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI

From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI
Date: Fri, 02 Sep 2011 11:05:13 -0500
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20110516 Lightning/1.0b2 Thunderbird/3.1.10

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<address@hidden>
   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 = 

@@ -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;
       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) {
     } else {
} else {

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.


Anthony Liguori


reply via email to

[Prev in Thread] Current Thread [Next in Thread]