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<aligu...@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.
Kevin
