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

[Max Reitz]

On 2015-01-28 at 16:01, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> >   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.

Thanks, I'll change it.

> >   ##
> > +# @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).

Yes, I'd rather drop 'force' here.

> 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.

Well, I'd rather call this interface deprecated from the start (it's 
basically as deprecated as 'change' is, only that 'change' is even worse 
because it unites multiple different commands) than making it suitable 
for any situation.

There are two points from my perspective: First, this is mainly a 
replacement for 'change'. Since 'change' did not have a 'force' 
parameter, I don't see the need to have one here either. Second, we can 
always add additional parameters later (without breaking compatibility); 
for instance, in this series still the read-only mode parameter will be 
added.

> > +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?

No, I got that file name from the 'change' example. :-)

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

No problem. I'll have to send a v2 anyway.

Max