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

[Kevin Wolf]

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 = 
>>> 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