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

[Kevin Wolf]

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