[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 14:19:37 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20110816 Thunderbird/6.0 |
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?
> 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?
> {
> BlockDriverState *bs;
> - int force = qdict_get_try_bool(qdict, "force", 0);
> - const char *filename = qdict_get_str(qdict, "device");
>
> - bs = bdrv_find(filename);
> + bs = bdrv_find(device);
> if (!bs) {
> - qerror_report(QERR_DEVICE_NOT_FOUND, filename);
> - return -1;
> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> + return;
> }
> - return eject_device(mon, bs, force);
> +
> + eject_device(bs, force, errp);
> }
Kevin
- [Qemu-devel] [PATCH 00/14] Convert commands to QAPI (batch 1), Anthony Liguori, 2011/08/24
- [Qemu-devel] [PATCH 01/14] qerror: add qerror_report_err(), Anthony Liguori, 2011/08/24
- [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