Re: [Qemu-devel] [PATCH v7 35/39] qmp: Introduce blockdev-change-medium

[Max Reitz <mreitz@redhat.com>]

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

On 23.10.2015 16:25, Kevin Wolf wrote:
> Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
>> 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-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.5
>> +##
>> +{ 'command': 'blockdev-change-medium',
>> +  'data': { 'device': 'str',
>> +            'filename': 'str',
>> +            '*format': 'str' } }
> 
> Do we really want to expose such an interface in a new QMP command? It
> isn't like blockdev-add, but like -hda. Which doesn't only mean that you
> can't specify most options, but also that filename is parsed for
> protocol names etc.

Let's go back to why this series exists.

Once upon a time, in the magical land of a certain IRC channel, there
was someone who wanted "change" to be able to change the R/W mode of the
medium.

The first iteration of the ensuing series had three patches and did just
that. Alas, however! some people were discontent. "change" is in a
horrible state, they said, Please separate the blockdev operation from
the VNC operation, they said.

And so the second iteration came around which added this here function,
blockdev-change-medium. Now that's much better!, they said. Alas! It was
not enough. Since you are already touching this, why don't you clean up
everything?, they asked.

And so the third iteration came into being, henceforth known as
"blockdev: BlockBackend and media" and re-numerated as v1, since it was
1250 % the size of the last version.

blockdev-change-medium, however, stayed. This is because change is such
an ugly beast that nobody should ever use it again, but the four new
atomic commands are too frightening for human users.

Oh, and it's also for the fact that this function pretty much exists
already and just isn't exposed to the outside yet.

And so this series has to this date been living happily ever after.


In short: It's been in this series since v0. A note next to "change"
tells people not to use it, but without this function here they have to
use it if they don't want to mess with the atomic commands. With this
function added, we can finally tell people *never* to use "change". So
that may be considered progress.

> Shouldn't new clients use blockdev-add and the separate tray-open/close
> and remove/insert-medium commands instead of converting from one bad
> commannd (change) to another (this one)?

Clients, yes, humans, no.

(Although I'll happily accept the obvious argument: Rarely any human
will ever use blockdev-change-medium over change. But at least we can
then tell them that it's simply wrong.)

> Or, if we really want to provide a convenience function, this should
> probably take a BlockdevRef instead of filename/format.

We want a change-like convenience function, i.e. one that takes a
filename. Just combining open-tray + remove-medium + insert-medium +
close-tray into a single command doesn't sound like too much convenience
to me. Creating the BDS is much more work than issuing all of these four
commands.

So I don't know whether to drop it. If I drop this function, we still
cannot fully deprecate change. Also, this function pretty much offers
itself, this patch changes only very little code.

I do see the "Do you really want to introduce a function that is going
to be legacy right from the start?" argument. But then again, we'll have
to support change anyway, so I don't think this will cost us anything.

I don't know.

Max


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