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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 24 Aug 2011 13:43:03 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> Signed-off-by: Anthony Liguori <aliguori@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(-)
> 
> 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)
>  {
>      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);
>  }
>  
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict,
> @@ -715,7 +715,7 @@ int do_change_block(Monitor *mon, const char *device,
>              return -1;
>          }
>      }
> -    if (eject_device(mon, bs, 0) < 0) {
> +    if (eject_device(bs, 0, NULL) < 0) {

This will make the change command return an undefined error for errors
caught in eject_device(). I believe this is fixed in patch 13/14? If yes,
the it's probably a good thing to note it in the commit log.

>          return -1;
>      }
>      bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> diff --git a/blockdev.h b/blockdev.h
> index 3587786..badbf01 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -58,7 +58,6 @@ DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
>  DriveInfo *add_init_drive(const char *opts);
>  
>  void do_commit(Monitor *mon, const QDict *qdict);
> -int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_block_set_passwd(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 0ccfb28..bcb789b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -76,8 +76,7 @@ ETEXI
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
>          .help       = "eject a removable medium (use -f to force it)",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_eject,
> +        .mhandler.cmd = hmp_eject,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 47e1ff7..36eb5b9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -24,3 +24,17 @@ void hmp_info_name(Monitor *mon)
>      }
>      qapi_free_NameInfo(info);
>  }
> +
> +void hmp_eject(Monitor *mon, const QDict *qdict)
> +{
> +    int force = qdict_get_try_bool(qdict, "force", 0);
> +    const char *device = qdict_get_str(qdict, "device");
> +    Error *err = NULL;
> +
> +    qmp_eject(device, true, force, &err);
> +    if (err) {
> +        monitor_printf(mon, "eject: %s\n", error_get_pretty(err));
> +        error_free(err);
> +    }
> +}
> +
> diff --git a/hmp.h b/hmp.h
> index 5fe73f1..6a552c1 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -18,5 +18,6 @@
>  #include "qapi-types.h"
>  
>  void hmp_info_name(Monitor *mon);
> +void hmp_eject(Monitor *mon, const QDict *args);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 654409b..934ea81 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -24,3 +24,28 @@
>  ##
>  { 'command': 'query-name', 'returns': 'NameInfo' }
>  
> +##
> +# @eject:
> +#
> +# Ejects a device from a removable drive.
> +#
> +# @device:  The name of the device
> +#
> +# @force:   @optional If true, eject regardless of whether the drive is locked.
> +#           If not specified, the default value is false.
> +#
> +# Returns:  Nothing on success
> +#           If @device is not a valid block device, DeviceNotFound
> +#           If @device is not removable and @force is false, DeviceNotRemovable
> +#           If @force is false and @device is locked, DeviceLocked
> +#
> +# Notes:    If the @force flag is used, the backing file will be closed
> +#           regardless of whether the device is removable.  This may result in
> +#           a badly broken guest.
> +#
> +#           Ejecting a device with no media results in success
> +#
> +# Since: 0.14.0
> +##

We're duplicating the documentation, as it also exists in qmp-commands.hx.

Should we drop it from there? If we do, we'll have to update the script
that generates QMP/qmp-commands.txt.

> +{ 'command': 'eject', 'data': {'device': 'str', '*force': 'bool'} }
> +
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 03f67da..81d1800 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -89,8 +89,7 @@ EQMP
>          .args_type  = "force:-f,device:B",
>          .params     = "[-f] device",
>          .help       = "eject a removable medium (use -f to force it)",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_eject,
> +        .mhandler.cmd_new = qmp_marshal_input_eject,
>      },
>  
>  SQMP