Re: [Qemu-block] [PATCH v7 31/39] blockdev: Add blockdev-insert-medium

[Kevin Wolf]

Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
> And a helper function for that, which directly takes a pointer to the
> BDS to be inserted instead of its node-name (which will be used for
> implementing 'change' using blockdev-insert-medium).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  blockdev.c           | 54 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 17 +++++++++++++++++
>  qmp-commands.hx      | 37 +++++++++++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a8601ca..a4c278f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2151,6 +2151,60 @@ void qmp_blockdev_remove_medium(const char *device, 
> Error **errp)
>      }
>  }
>  
> +static void qmp_blockdev_insert_anon_medium(const char *device,
> +                                            BlockDriverState *bs, Error 
> **errp)
> +{
> +    BlockBackend *blk;
> +    bool has_device;
> +
> +    blk = blk_by_name(device);
> +    if (!blk) {
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "Device '%s' not found", device);
> +        return;
> +    }
> +
> +    /* For BBs without a device, we can exchange the BDS tree at will */
> +    has_device = blk_get_attached_dev(blk);
> +
> +    if (has_device && !blk_dev_has_removable_media(blk)) {
> +        error_setg(errp, "Device '%s' is not removable", device);
> +        return;
> +    }
> +
> +    if (has_device && !blk_dev_is_tray_open(blk)) {
> +        error_setg(errp, "Tray of device '%s' is not open", device);
> +        return;
> +    }
> +
> +    if (blk_bs(blk)) {
> +        error_setg(errp, "There already is a medium in device '%s'", device);
> +        return;
> +    }
> +
> +    blk_insert_bs(blk, bs);
> +}
> +
> +void qmp_blockdev_insert_medium(const char *device, const char *node_name,
> +                                Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find_node(node_name);

Shouldn't this use bdrv_lookup_bs() to accept a BB name and be consisent
with most other commands? Of course, if you actually used a BB name, it
would fail below, but not with a confusing "not found" message.

> +    if (!bs) {
> +        error_setg(errp, "Node '%s' not found", node_name);
> +        return;
> +    }
> +
> +    if (bs->blk) {
> +        error_setg(errp, "Node '%s' is already in use by '%s'", node_name,
> +                   blk_name(bs->blk));
> +        return;
> +    }
> +
> +    qmp_blockdev_insert_anon_medium(device, bs, errp);
> +}

Kevin