| Message ID | 1422387983-32153-46-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 <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. 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
diff --git a/blockdev.c b/blockdev.c index 6e440b8..2ada2b1 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2012,8 +2012,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; @@ -2036,7 +2037,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) { drv = bdrv_find_whitelisted_format(format, bdrv_flags & BDRV_O_RDWR); if (!drv) { error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index 09d1e30..2a34332 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -65,8 +65,6 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type); DriveInfo *add_init_drive(const char *opts); -void qmp_change_blockdev(const char *device, const char *filename, - const char *format, Error **errp); void do_commit(Monitor *mon, const QDict *qdict); int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); #endif diff --git a/qapi-schema.json b/qapi-schema.json index e16f8eb..61867e1 100644 --- a/qapi-schema.json +++ 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). # # Since: 0.14.0 ## diff --git a/qapi/block-core.json b/qapi/block-core.json index ba41015..d3c3ca7 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1785,6 +1785,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.3 +## +{ '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 604d638..1987a09 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -3854,6 +3854,37 @@ Example: EQMP { + .name = "blockdev-change-medium", + .args_type = "device:B,filename:F,format:s?", + .mhandler.cmd_new = qmp_marshal_input_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_input_query_memdev, diff --git a/qmp.c b/qmp.c index 635d001..1cfdb74 100644 --- a/qmp.c +++ b/qmp.c @@ -417,7 +417,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 | 3 ++- qapi/block-core.json | 23 +++++++++++++++++++++++ qmp-commands.hx | 31 +++++++++++++++++++++++++++++++ qmp.c | 2 +- 6 files changed, 61 insertions(+), 7 deletions(-)