| Message ID | 1445270025-22999-36-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. 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)? Or, if we really want to provide a convenience function, this should probably take a BlockdevRef instead of filename/format. Kevin
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
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> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/blockdev.c b/blockdev.c index 50e5e74..db789e2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2094,8 +2094,9 @@ void qmp_blockdev_insert_medium(const char *device, const char *node_name, qmp_blockdev_insert_anon_medium(device, bs, errp); } -void qmp_change_blockdev(const char *device, const char *filename, - const char *format, Error **errp) +void qmp_blockdev_change_medium(const char *device, const char *filename, + bool has_format, const char *format, + Error **errp) { BlockBackend *blk; BlockBackendRootState *blk_rs; @@ -2119,7 +2120,7 @@ void qmp_change_blockdev(const char *device, const char *filename, bdrv_flags = blk_rs->read_only ? 0 : BDRV_O_RDWR; bdrv_flags |= blk_rs->open_flags & ~BDRV_O_RDWR; - if (format) { + if (has_format) { options = qdict_new(); qdict_put(options, "driver", qstring_from_str(format)); } diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index a00be94..b06a060 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -63,8 +63,6 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type); /* device-hotplug */ -void qmp_change_blockdev(const char *device, const char *filename, - const char *format, Error **errp); void hmp_commit(Monitor *mon, const QDict *qdict); void hmp_drive_del(Monitor *mon, const QDict *qdict); #endif diff --git a/qapi-schema.json b/qapi-schema.json index 702b7b5..058b8ec 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1842,8 +1842,10 @@ # device's password. The behavior of reads and writes to the block # 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. +# Notes: 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. # # Since: 0.14.0 ## diff --git a/qapi/block-core.json b/qapi/block-core.json index 81a1f19..b8cc18a 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1949,6 +1949,29 @@ ## +# @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' } } + + +## # @BlockErrorAction # # An enumeration of action that has been taken when a DISK I/O occurs diff --git a/qmp-commands.hx b/qmp-commands.hx index 16d7e2a..8643d95 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -4154,6 +4154,37 @@ Example: EQMP { + .name = "blockdev-change-medium", + .args_type = "device:B,filename:F,format:s?", + .mhandler.cmd_new = qmp_marshal_blockdev_change_medium, + }, + +SQMP +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. + +Arguments: + +- "device": device name (json-string) +- "filename": filename of the new image (json-string) +- "format": format of the new image (json-string, optional) + +Examples: + +1. Change a removable medium + +-> { "execute": "blockdev-change-medium", + "arguments": { "device": "ide1-cd0", + "filename": "/srv/images/Fedora-12-x86_64-DVD.iso", + "format": "raw" } } +<- { "return": {} } + +EQMP + + { .name = "query-memdev", .args_type = "", .mhandler.cmd_new = qmp_marshal_query_memdev, diff --git a/qmp.c b/qmp.c index ff54e5a..4e44f98 100644 --- a/qmp.c +++ b/qmp.c @@ -414,7 +414,7 @@ void qmp_change(const char *device, const char *target, if (strcmp(device, "vnc") == 0) { qmp_change_vnc(target, has_arg, arg, errp); } else { - qmp_change_blockdev(device, target, arg, errp); + qmp_blockdev_change_medium(device, target, has_arg, arg, errp); } }
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 | 6 ++++-- qapi/block-core.json | 23 +++++++++++++++++++++++ qmp-commands.hx | 31 +++++++++++++++++++++++++++++++ qmp.c | 2 +- 6 files changed, 63 insertions(+), 8 deletions(-)