Re: [Qemu-devel] [PATCH RESEND 40/50] blockdev: Add blockdev-remove-medium

[Eric Blake]

On 01/27/2015 12:46 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  blockdev.c           | 25 +++++++++++++++++++++++++
>  qapi/block-core.json | 13 +++++++++++++
>  qmp-commands.hx      | 43 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+)
> 

>  
> +void qmp_blockdev_remove_medium(const char *device, Error **errp)
> +{

> +    if (!blk_dev_is_tray_open(blk)) {
> +        error_setg(errp, "Tray of device '%s' is not open", device);
> +        return;
> +    }
> +
> +    if (blk_bs(blk)) {
> +        blk_remove_bs(blk);

Another intentional no-op if there is already no medium; worth
documenting alongside the other touchups.

>  
> +##
> +# @blockdev-remove-medium:
> +#
> +# Removes a medium (a block driver state tree) from a block device. That 
> block
> +# device's tray must currently be open.
> +#
> +# @device: block device name
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'blockdev-remove-medium',
> +  'data': { 'device': 'str' } }

Just thinking aloud - obviously, this is a device operation.  But do we
want to allow specifying the device by the node-name of the BDS being
removed?  (I suppose the same applies to 38 [open a tray by the name of
the BDS in the tray] and 39 [close a tray that has the given BDS
inserted]).  But I'm fine with naming the parameter 'device', even if we
allow for a BDS->BB lookup when actually resolving the user's input,
since that would only be a convenience (and not like other block API
that specifically operate on nodes of a BDS tree rather than a device).

Furthermore, the counterpart command for inserting a medium (later in
this series) is one case where we CAN'T do the BDS->BB lookup
(generally, insertion will fail if a BDS node is already in the BB
device, unless you implement swap semantics, but that would make it
confusing to insert one BDS into the device referenced by another
BDS->BB lookup); and symmetry argues that if that command supports ONLY
a BB name, then all of the related commands are just fine using 'device'
as their parameter name to imply BB name.

So I'm fine with the naming you've used so far.

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