Re: [Qemu-devel] [PATCH RESEND 45/50] qmp: Introduce blockdev-change-medium

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3168 bytes --]

On 01/27/2015 12:46 PM, Max Reitz wrote:
> Introduce a new QMP command 'blockdev-change-medium' which is intended
> to replace the 'change' command for block devices. The existing function
> qmp_change_blockdev() is accordingly renamed to
> qmp_blockdev_change_medium().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c                |  7 ++++---
>  include/sysemu/blockdev.h |  2 --
>  qapi-schema.json          |  3 ++-
>  qapi/block-core.json      | 23 +++++++++++++++++++++++
>  qmp-commands.hx           | 31 +++++++++++++++++++++++++++++++
>  qmp.c                     |  2 +-
>  6 files changed, 61 insertions(+), 7 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -1649,7 +1649,8 @@
>  #          device between when these calls are executed is undefined.
>  #
>  # Notes:  It is strongly recommended that this interface is not used especially
> -#         for changing block devices.
> +#         for changing block devices.  Please use blockdev-change-medium
> +#         instead (for VNC, please use change-vnc-password).

Not grammatically wrong, but still feels a bit awkward.  Maybe better
worded as:

This interface is deprecated, and it is strongly recommended that you
avoid using it.  For changing block devices, use blockdev-change-medium;
for changing VNC parameters, use change-vnc-password.


>  ##
> +# @blockdev-change-medium:
> +#
> +# Changes the medium inserted into a block device by ejecting the current medium
> +# and loading a new image file which is inserted as the new medium (this command
> +# combines blockdev-open-tray, blockdev-remove-medium, blockdev-insert-medium
> +# and blockdev-close-tray).
> +#
> +# @device:      block device name
> +#
> +# @filename:    filename of the new image to be loaded
> +#
> +# @format:      #optional, format to open the new image with (defaults to the
> +#               probed format)
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'blockdev-change-medium',
> +  'data': { 'device': 'str',
> +            'filename': 'str',
> +            '*format': 'str' } }

Intentional that there is no way to specify 'force'?  I can live with
that (force is a sledgehammer; and someone that can justify using it can
just use the lower-level functions themselves.  No need to bloat the
nice wrapper interface with something that is usually not needed).

I'm unclear on whether this command will wait for the tray open to
happen, or if it fails in the middle if the tray was locked and a
request sent to the guest but the guest did not act fast enough to
unlock things.  As such, I'm not quite sure if this interface is missing
any parameters.


> +Examples:
> +
> +1. Change a removable medium
> +
> +-> { "execute": "blockdev-change-medium",
> +             "arguments": { "device": "ide1-cd0",
> +                            "filename": "/srv/images/Fedora-12-x86_64-DVD.iso",

Wow - you're still testing a Fedora 12 image?

I'm a bit reluctant to mark this one reviewed, without more discussion.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]