Re: [Qemu-devel] [PATCH RESEND 41/50] blockdev: Add blockdev-insert-medium

[Eric Blake]

On 01/27/2015 12:46 PM, Max Reitz wrote:
> 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           | 43 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 17 +++++++++++++++++
>  qmp-commands.hx      | 38 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+)
> 

> +static void qmp_blockdev_insert_anon_medium(const char *device,
> +                                            BlockDriverState *bs, Error 
> **errp)
> +{
> +    BlockBackend *blk;
> +
> +    blk = blk_by_name(device);
> +    if (!blk) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!blk_dev_has_removable_media(blk)) {
> +        error_setg(errp, "Device '%s' is not removable", device);
> +        return;
> +    }
> +
> +    if (!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;
> +    }

Good, you didn't implement hot-swap semantics of replacing an existing
medium (that gets too confusing; so I _like_ that you force the user to
consider all the steps through multiple low-level commands).


> +
> +Example (1):

I'll quit pointing it out; but if you clean up the useless (1) in one
patch, do it across the series.

> +
> +-> { "execute": "blockdev-add",
> +     "arguments": { "options": { "id": "backend0",
> +                                 "node-name": "node0",

Why is 'id' needed?  Isn't the point of this command sequence to create
a BDS tree that is NOT tied to a BB, and then use insert-medium to make
the association after the fact?  We don't need to create a BB named
'backend0' if we are immediately going to reuse 'node0' in a different
BB (true, we have somewhat anticipated the idea of sharing BDS tree
among multiple BB, but haven't quite turned that on before now).

> +                                 "driver": "raw",
> +                                 "file": { "driver": "file",
> +                                           "filename": "fedora.iso" } } } }
> +
> +<- { "return": {} }
> +
> +-> { "execute": "blockdev-insert-medium",
> +     "arguments": { "device": "ide1-cd0",
> +                    "node-name": "node0" } }
> +
> +<- { "return": {} }

If you can either explain why you used 'id' in the example, or remove
that parameter, you can add:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature