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

[Anthony Liguori <anthony@codemonkey.ws>]

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<aliguori@us.ibm.com>
>>>> ---
>>>>    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
>