Re: [Qemu-devel] [PATCH v2] RFC: Add blockdev-del QMP command

[Fam Zheng <famz@redhat.com>]

On Wed, 02/12 09:36, Ian Main wrote:
> This is the sister command to blockdev-add.  In Fam's example he uses
> the drive_del HMP command to clean up but it would be much nicer to
> have a way to do this via QMP.
> 
> Signed-off-by: Ian Main <imain@redhat.com>

Thank you for doing this!

> ---
> 
> v2:
> - s/blockdev-delete/blockdev-del
> - Fixed example.
> 
>  blockdev.c       | 52 ++++++++++++++++++++++++++++++++++++++--------------
>  qapi-schema.json | 11 +++++++++++
>  qmp-commands.hx  | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+), 14 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 7372721..96d0da1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1733,21 +1733,9 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>      }
>  }
>  
> -int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +/* This is called by both do_drive_del() and qmp_blockdev_del */
> +static int drive_del_core(BlockDriverState *bs)
>  {
> -    const char *id = qdict_get_str(qdict, "id");
> -    BlockDriverState *bs;
> -
> -    bs = bdrv_find(id);
> -    if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> -        return -1;
> -    }
> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
> -        qerror_report(QERR_DEVICE_IN_USE, id);
> -        return -1;
> -    }
> -
>      /* quiesce block driver; prevent further io */
>      bdrv_drain_all();
>      bdrv_flush(bs);
> @@ -1771,6 +1759,25 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>      return 0;
>  }
>  
> +int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *id = qdict_get_str(qdict, "id");
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(id);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> +        return -1;
> +    }
> +
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
> +        qerror_report(QERR_DEVICE_IN_USE, id);
> +        return -1;
> +    }
> +
> +    return drive_del_core(bs);
> +}
> +
>  void qmp_block_resize(bool has_device, const char *device,
>                        bool has_node_name, const char *node_name,
>                        int64_t size, Error **errp)
> @@ -2386,6 +2393,23 @@ fail:
>      qmp_output_visitor_cleanup(ov);
>  }
>  
> +void qmp_blockdev_del(const char *device, Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_setg(errp, "Block device not found");
> +        return;
> +    }
> +
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, NULL)) {
> +        return;
> +    }
> +
> +    drive_del_core(bs);
> +}
> +

We could drop drive_del_core and move everything into qmp_blockdev_del().

In the original do_drive_del (maybe rename it to hmp_drive_del as a better
name?), call qmp_blockdev_del with a local_err, and report error with
qerror_report_err().

Thanks,
Fam