[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: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI |
Date: |
Thu, 25 Aug 2011 15:52:52 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20110816 Thunderbird/6.0 |
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
- Re: [Qemu-devel] [PATCH 01/14] qerror: add qerror_report_err(), (continued)
- [Qemu-devel] [PATCH 07/14] qerror: add additional parameter to QERR_DEVICE_ENCRYPTED, Anthony Liguori, 2011/08/24
- [Qemu-devel] [PATCH 05/14] block: add unsafe_probe, Anthony Liguori, 2011/08/24
- [Qemu-devel] [PATCH 11/14] qapi: add change-vnc-listen, Anthony Liguori, 2011/08/24
- [Qemu-devel] [PATCH 06/14] monitor: expose readline state, Anthony Liguori, 2011/08/24
- [Qemu-devel] [PATCH 08/14] qapi: convert eject (qmp and hmp) to QAPI, Anthony Liguori, 2011/08/24
[Qemu-devel] [PATCH 02/14] qapi: add code generation support for middle mode, Anthony Liguori, 2011/08/24
[Qemu-devel] [PATCH 09/14] qapi: convert block_passwd and add set-blockdev-password, Anthony Liguori, 2011/08/24
[Qemu-devel] [PATCH 04/14] qapi: convert query-name, Anthony Liguori, 2011/08/24
[Qemu-devel] [PATCH 10/14] qapi: add change-vnc-password, Anthony Liguori, 2011/08/24