[Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Max Reitz]

The 'change' QMP and HMP command allows replacing the medium in drives
which support this, e.g. floppy disk drives. For some drives, the medium
carries information about whether it can be written to or not (again,
floppy drives). Therefore, it should be possible to change the read-only
state of block devices when changing the loaded medium.

Following a suggestion from Eric, this series first introduces a
'blockdev-change-medium' QMP command which is intended to replace the
'change' command for block devices. Then, an optional additional
'read-only' parameter is added which allows chaning the read-only state
in three ways:

- 'retain': Just keep the status as it was before; this is the current
  behavior and thus this will be the default.
- 'ro': Force read-only access
- 'rw': Force writable access

Finally, that 'read-only' parameter is added to the HMP 'change'
command. This series does not add a 'blockdev-change-medium' QMP command
because 'change' being overloaded for VNC and block devices is not too
bad for HMP (while it is for QMP).


v2:
- basically completely rewritten
- Dropped 'auto' [Kevin and Markus]
- Introduced blockdev-change-medium [Eric]

- Patch 1 introduces the new QMP command 'blockdev-change-medium'; there
  are (at least) two questionable design choices which I want to explain
  here:
  - The name is rather long; furthermore, the name 'change-blockdev' was
    already suggested by the existing code. I used such a long name
    because (1) there are no *-blockdev commands, but there are
    blockdev-* commands, so "blockdev" should be the prefix, not the
    suffix, and (2) "blockdev-change" could mean anything, so I wanted
    to be as clear as possible.
  - The 'format' argument is optional; this is because by making it
    mandatory, it would have been difficult for the 'change' QMP and HMP
    commands to retain their 'format' argument optional as well (which
    we have to do thanks to compatibility)
- Patches 2 to 4 should be thoroughly explained by their commit title


Max Reitz (4):
  qmp: Introduce blockdev-change-medium
  hmp: Use blockdev-change-medium for change command
  blockdev: Add read-only option to blockdev-change-medium
  hmp: Add read-only option to change command

 blockdev.c                | 31 +++++++++++++++++++++++----
 hmp-commands.hx           | 20 +++++++++++++++---
 hmp.c                     | 46 +++++++++++++++++++++++++++++-----------
 include/sysemu/blockdev.h |  2 --
 qapi/block-core.json      | 43 ++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx           | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 qmp.c                     |  3 ++-
 7 files changed, 176 insertions(+), 22 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/4] qmp: Introduce blockdev-change-medium

[Max Reitz]

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/block-core.json      | 21 +++++++++++++++++++++
 qmp-commands.hx           | 31 +++++++++++++++++++++++++++++++
 qmp.c                     |  2 +-
 5 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5651a8e..e8947a5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1768,8 +1768,9 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
     }
 }
 
-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;
     BlockDriverState *bs;
@@ -1788,7 +1789,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (format) {
+    if (has_format) {
         drv = bdrv_find_whitelisted_format(format, bs->read_only);
         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/block-core.json b/qapi/block-core.json
index 6e8db15..431517d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1662,6 +1662,27 @@
 
 
 ##
+# @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.
+#
+# @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 d4b0010..3f1f2eb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3646,6 +3646,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 0b4f131..59fc6f7 100644
--- a/qmp.c
+++ b/qmp.c
@@ -402,7 +402,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);
     }
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 1/4] qmp: Introduce blockdev-change-medium

[Eric Blake]

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

On 12/05/2014 03:08 AM, 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/block-core.json      | 21 +++++++++++++++++++++
>  qmp-commands.hx           | 31 +++++++++++++++++++++++++++++++
>  qmp.c                     |  2 +-
>  5 files changed, 57 insertions(+), 6 deletions(-)

I like 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.
> +#
> +# @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' } }

Someday, we should quit open-coding format as 'str' and instead make it
an enum; but that is a separate patch series, and can get this location
as part of that series.

Should there be a way to empty a cdrom drive?  That is, how do I change
a drive from visiting a file to being empty?  Does passing the empty
string for mandatory 'filename' do that, or should we be strict and
state that filename is optional (omit to empty the drive) and that an
empty string as filename is forbidden?

Please also improve the documentation in qapi-schema.json to have the
'change' command mention that it is kept for backwards compatibility,
but that it is no longer the preferred command and point users to
'blockdev-change-medium'.

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 1/4] qmp: Introduce blockdev-change-medium

[Max Reitz]

On 2014-12-05 at 14:10, Eric Blake wrote:
> On 12/05/2014 03:08 AM, 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/block-core.json      | 21 +++++++++++++++++++++
>>   qmp-commands.hx           | 31 +++++++++++++++++++++++++++++++
>>   qmp.c                     |  2 +-
>>   5 files changed, 57 insertions(+), 6 deletions(-)
> I like it!

Good :-)

>>   
>>   ##
>> +# @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.
>> +#
>> +# @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' } }
> Someday, we should quit open-coding format as 'str' and instead make it
> an enum; but that is a separate patch series, and can get this location
> as part of that series.
>
> Should there be a way to empty a cdrom drive?  That is, how do I change
> a drive from visiting a file to being empty?

Doesn't 'eject' work?

> Does passing the empty
> string for mandatory 'filename' do that, or should we be strict and
> state that filename is optional (omit to empty the drive) and that an
> empty string as filename is forbidden?
>
> Please also improve the documentation in qapi-schema.json to have the
> 'change' command mention that it is kept for backwards compatibility,
> but that it is no longer the preferred command and point users to
> 'blockdev-change-medium'.

And to 'change-vnc-password'. Will do.

Max

Re: [Qemu-devel] [PATCH v2 1/4] qmp: Introduce blockdev-change-medium

[Eric Blake]

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

On 12/05/2014 06:18 AM, Max Reitz wrote:
>>
>> Should there be a way to empty a cdrom drive?  That is, how do I change
>> a drive from visiting a file to being empty?
> 
> Doesn't 'eject' work?

Oh, good point. More cross-referencing documentation work, then :)

At which point, THIS command can keep a mandatory 'filename', and should
probably fail on "" rather than being a synonym to 'eject'.

> 
>> Does passing the empty
>> string for mandatory 'filename' do that, or should we be strict and
>> state that filename is optional (omit to empty the drive) and that an
>> empty string as filename is forbidden?
>>
>> Please also improve the documentation in qapi-schema.json to have the
>> 'change' command mention that it is kept for backwards compatibility,
>> but that it is no longer the preferred command and point users to
>> 'blockdev-change-medium'.
> 
> And to 'change-vnc-password'. Will do.
> 
> Max
> 
> 

-- 
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: 539 bytes --]

[Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command

[Max Reitz]

Use separate code paths for the two overloaded functions of the 'change'
HMP command, and invoke the 'blockdev-change-medium' QMP command if used
on a block device (by calling qmp_blockdev_change_medium()).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hmp.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index 481be80..ef5e8bd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1179,22 +1179,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     const char *arg = qdict_get_try_str(qdict, "arg");
     Error *err = NULL;
 
-    if (strcmp(device, "vnc") == 0 &&
-            (strcmp(target, "passwd") == 0 ||
-             strcmp(target, "password") == 0)) {
-        if (!arg) {
-            monitor_read_password(mon, hmp_change_read_arg, NULL);
+    if (strcmp(device, "vnc") == 0) {
+        if (strcmp(target, "passwd") == 0 ||
+            strcmp(target, "password") == 0) {
+            if (!arg) {
+                monitor_read_password(mon, hmp_change_read_arg, NULL);
+                return;
+            }
+        }
+        qmp_change("vnc", target, !!arg, arg, &err);
+    } else {
+        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
+        if (err &&
+            error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
+            error_free(err);
+            monitor_read_block_device_key(mon, device, NULL, NULL);
             return;
         }
     }
 
-    qmp_change(device, target, !!arg, arg, &err);
-    if (err &&
-        error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
-        error_free(err);
-        monitor_read_block_device_key(mon, device, NULL, NULL);
-        return;
-    }
     hmp_handle_error(mon, &err);
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command

[Eric Blake]

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

On 12/05/2014 03:08 AM, Max Reitz wrote:
> Use separate code paths for the two overloaded functions of the 'change'
> HMP command, and invoke the 'blockdev-change-medium' QMP command if used
> on a block device (by calling qmp_blockdev_change_medium()).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  hmp.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> +    } else {
> +        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
> +        if (err &&
> +            error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
> +            error_free(err);
> +            monitor_read_block_device_key(mon, device, NULL, NULL);
>              return;

Hmm. Not a problem with this patch (which is just reorganizing control
flow), but a possible design issue.  The old 'change' QMP command cannot
provide the encryption code, hence it cannot succeed when changing to an
encrypted media.  But now that we are adding a new QMP command, we could
possibly rectify that matter.  However, the way I'm envisioning that is
that blockdev-change-medium gains an optional parameter for encryption
key.  Then the HMP command gains a new optional parameter for specifying
the key, and the logic flow would look a bit more like:

qmp_blockdev_change_medium(device, target, !!arg, arg,
                           !!key, key, &err);
if (err && error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED &&
    !key) {
    error_free(err);
    key = prompt_for_key_from_monitor;
    qmp_blockdev_change_medium(device, target, !!arg, arg,
                               true, key, &err);
}

which would mean that a QMP command can now supply the key in one
command, rather than HMP being the only way to supply a password because
of how it does a two-step process (that is,
monitor_read_block_device_key isn't really accessible from QMP).

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command

[Max Reitz]

On 2014-12-05 at 14:24, Eric Blake wrote:
> On 12/05/2014 03:08 AM, Max Reitz wrote:
>> Use separate code paths for the two overloaded functions of the 'change'
>> HMP command, and invoke the 'blockdev-change-medium' QMP command if used
>> on a block device (by calling qmp_blockdev_change_medium()).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   hmp.c | 27 +++++++++++++++------------
>>   1 file changed, 15 insertions(+), 12 deletions(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +    } else {
>> +        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
>> +        if (err &&
>> +            error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
>> +            error_free(err);
>> +            monitor_read_block_device_key(mon, device, NULL, NULL);
>>               return;
> Hmm. Not a problem with this patch (which is just reorganizing control
> flow), but a possible design issue.  The old 'change' QMP command cannot
> provide the encryption code, hence it cannot succeed when changing to an
> encrypted media.  But now that we are adding a new QMP command, we could
> possibly rectify that matter.  However, the way I'm envisioning that is
> that blockdev-change-medium gains an optional parameter for encryption
> key.  Then the HMP command gains a new optional parameter for specifying
> the key, and the logic flow would look a bit more like:
>
> qmp_blockdev_change_medium(device, target, !!arg, arg,
>                             !!key, key, &err);
> if (err && error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED &&
>      !key) {
>      error_free(err);
>      key = prompt_for_key_from_monitor;
>      qmp_blockdev_change_medium(device, target, !!arg, arg,
>                                 true, key, &err);
> }
>
> which would mean that a QMP command can now supply the key in one
> command, rather than HMP being the only way to supply a password because
> of how it does a two-step process (that is,
> monitor_read_block_device_key isn't really accessible from QMP).

I'd rather put all of the encryption stuff back until we have figured 
out how to fix it (that is, if we want to fix it at all)...

Max

Re: [Qemu-devel] [PATCH v2 2/4] hmp: Use blockdev-change-medium for change command

[Eric Blake]

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

On 12/05/2014 06:27 AM, Max Reitz wrote:
>> Hmm. Not a problem with this patch (which is just reorganizing control
>> flow), but a possible design issue.  The old 'change' QMP command cannot
>> provide the encryption code, hence it cannot succeed when changing to an
>> encrypted media.  But now that we are adding a new QMP command, we could
>> possibly rectify that matter.  However, the way I'm envisioning that is
>> that blockdev-change-medium gains an optional parameter for encryption
>> key.  Then the HMP command gains a new optional parameter for specifying
>> the key, and the logic flow would look a bit more like:
>>
>> qmp_blockdev_change_medium(device, target, !!arg, arg,
>>                             !!key, key, &err);
>> if (err && error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED &&
>>      !key) {
>>      error_free(err);
>>      key = prompt_for_key_from_monitor;
>>      qmp_blockdev_change_medium(device, target, !!arg, arg,
>>                                 true, key, &err);
>> }
>>
>> which would mean that a QMP command can now supply the key in one
>> command, rather than HMP being the only way to supply a password because
>> of how it does a two-step process (that is,
>> monitor_read_block_device_key isn't really accessible from QMP).
> 
> I'd rather put all of the encryption stuff back until we have figured
> out how to fix it (that is, if we want to fix it at all)...

Yeah, I totally understand (no point adding encryption unless we add it
right).  Which is why it doesn't hold up this series.  But I _do_ want
to make sure we're thinking through the design, and not painting
ourselves into a corner; so at this point, I guess I'm just looking for
confirmation on my thought experiment that we are extensible enough that
we can support encryption at the QMP level down the road by adding
optional parameters, regardless of how we map that at the HMP level.


-- 
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: 539 bytes --]

[Qemu-devel] [PATCH v2 3/4] blockdev: Add read-only option to blockdev-change-medium

[Max Reitz]

Add an option to qmp_blockdev_change_medium() which allows changing the
read-only status of the block device whose medium is changed.

Some drives do not have a inherently fixed read-only status; for
instance, floppy disks can be set read-only or writable independently of
the drive. Some users may find it useful to be able to therefore change
the read-only status of a block device when changing the medium.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           | 24 +++++++++++++++++++++++-
 hmp.c                |  2 +-
 qapi/block-core.json | 24 +++++++++++++++++++++++-
 qmp-commands.hx      | 24 +++++++++++++++++++++++-
 qmp.c                |  3 ++-
 5 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e8947a5..84b2578 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1770,6 +1770,8 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
 
 void qmp_blockdev_change_medium(const char *device, const char *filename,
                                 bool has_format, const char *format,
+                                bool has_read_only,
+                                BlockdevChangeReadOnlyMode read_only,
                                 Error **errp)
 {
     BlockBackend *blk;
@@ -1803,7 +1805,27 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto out;
     }
 
-    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+    if (!has_read_only) {
+        read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
+    }
+
+    switch (read_only) {
+    case BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN:
+        bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+        break;
+
+    case BLOCKDEV_CHANGE_READ_ONLY_MODE_RO:
+        bdrv_flags = 0;
+        break;
+
+    case BLOCKDEV_CHANGE_READ_ONLY_MODE_RW:
+        bdrv_flags = BDRV_O_RDWR;
+        break;
+
+    default:
+        abort();
+    }
+
     bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
 
     qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
diff --git a/hmp.c b/hmp.c
index ef5e8bd..840da5c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1189,7 +1189,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
         }
         qmp_change("vnc", target, !!arg, arg, &err);
     } else {
-        qmp_blockdev_change_medium(device, target, !!arg, arg, &err);
+        qmp_blockdev_change_medium(device, target, !!arg, arg, false, 0, &err);
         if (err &&
             error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
             error_free(err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 431517d..f58f446 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1662,6 +1662,24 @@
 
 
 ##
+# @BlockdevChangeReadOnlyMode:
+#
+# Specifies the new read-only mode of a block device subject to the
+# @blockdev-change-medium command.
+#
+# @retain:  Retains the current read-only mode
+#
+# @ro:      Makes the device read-only
+#
+# @rw:      Makes the device writable
+#
+# Since: 2.3
+##
+{ 'enum': 'BlockdevChangeReadOnlyMode',
+  'data': ['retain', 'ro', 'rw'] }
+
+
+##
 # @blockdev-change-medium:
 #
 # Changes the medium inserted into a block device by ejecting the current medium
@@ -1674,12 +1692,16 @@
 # @format:      #optional, format to open the new image with (defaults to the
 #               probed format)
 #
+# @read-only:   #optional, change the read-only mode of the device; defaults to
+#               'retain'
+#
 # Since: 2.3
 ##
 { 'command': 'blockdev-change-medium',
   'data': { 'device': 'str',
             'filename': 'str',
-            '*format': 'str' } }
+            '*format': 'str',
+            '*read-only': 'BlockdevChangeReadOnlyMode' } }
 
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3f1f2eb..325db61 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3647,7 +3647,7 @@ EQMP
 
     {
         .name       = "blockdev-change-medium",
-        .args_type  = "device:B,filename:F,format:s?",
+        .args_type  = "device:B,filename:F,format:s?,read-only:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_change_medium,
     },
 
@@ -3663,6 +3663,8 @@ Arguments:
 - "device": device name (json-string)
 - "filename": filename of the new image (json-string)
 - "format": format of the new image (json-string, optional)
+- "read-only": new read-only mode (json-string, optional)
+          - Possible values: "retain" (default), "ro", "rw"
 
 Examples:
 
@@ -3674,6 +3676,26 @@ Examples:
                             "format": "raw" } }
 <- { "return": {} }
 
+2. Load a read-only medium into a writable drive
+
+-> { "execute": "blockdev-change-medium",
+             "arguments": { "device": "isa-fd0",
+                            "filename": "/srv/images/ro.img",
+                            "format": "raw",
+                            "read-only": "retain" } }
+
+<- { "error":
+     { "class": "GenericError",
+       "desc": "Could not open '/srv/images/ro.img': Permission denied" } }
+
+-> { "execute": "blockdev-change-medium",
+             "arguments": { "device": "isa-fd0",
+                            "filename": "/srv/images/ro.img",
+                            "format": "raw",
+                            "read-only": "ro" } }
+
+<- { "return": {} }
+
 EQMP
 
     {
diff --git a/qmp.c b/qmp.c
index 59fc6f7..febc1f6 100644
--- a/qmp.c
+++ b/qmp.c
@@ -402,7 +402,8 @@ void qmp_change(const char *device, const char *target,
     if (strcmp(device, "vnc") == 0) {
         qmp_change_vnc(target, has_arg, arg, errp);
     } else {
-        qmp_blockdev_change_medium(device, target, has_arg, arg, errp);
+        qmp_blockdev_change_medium(device, target, has_arg, arg, false, 0,
+                                   errp);
     }
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 3/4] blockdev: Add read-only option to blockdev-change-medium

[Eric Blake]

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

On 12/05/2014 03:08 AM, Max Reitz wrote:
> Add an option to qmp_blockdev_change_medium() which allows changing the
> read-only status of the block device whose medium is changed.
> 
> Some drives do not have a inherently fixed read-only status; for
> instance, floppy disks can be set read-only or writable independently of
> the drive. Some users may find it useful to be able to therefore change
> the read-only status of a block device when changing the medium.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c           | 24 +++++++++++++++++++++++-
>  hmp.c                |  2 +-
>  qapi/block-core.json | 24 +++++++++++++++++++++++-
>  qmp-commands.hx      | 24 +++++++++++++++++++++++-
>  qmp.c                |  3 ++-
>  5 files changed, 72 insertions(+), 5 deletions(-)

Feels much nicer than v1 :)

> +2. Load a read-only medium into a writable drive
> +
> +-> { "execute": "blockdev-change-medium",
> +             "arguments": { "device": "isa-fd0",
> +                            "filename": "/srv/images/ro.img",
> +                            "format": "raw",
> +                            "read-only": "retain" } }
> +
> +<- { "error":
> +     { "class": "GenericError",
> +       "desc": "Could not open '/srv/images/ro.img': Permission denied" } }
> +
> +-> { "execute": "blockdev-change-medium",
> +             "arguments": { "device": "isa-fd0",
> +                            "filename": "/srv/images/ro.img",
> +                            "format": "raw",
> +                            "read-only": "ro" } }
> +
> +<- { "return": {} }

Nice example.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 539 bytes --]

[Qemu-devel] [PATCH v2 4/4] hmp: Add read-only option to change command

[Max Reitz]

Expose the new read-only option of 'blockdev-change-medium' for the
'change' HMP command.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hmp-commands.hx | 20 +++++++++++++++++---
 hmp.c           | 21 ++++++++++++++++++++-
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..70cb7d7 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -196,8 +196,8 @@ ETEXI
 
     {
         .name       = "change",
-        .args_type  = "device:B,target:F,arg:s?",
-        .params     = "device filename [format]",
+        .args_type  = "device:B,target:F,arg:s?,read-only:s?",
+        .params     = "device filename [format [read-only]]",
         .help       = "change a removable medium, optional format",
         .mhandler.cmd = hmp_change,
     },
@@ -209,7 +209,7 @@ STEXI
 Change the configuration of a device.
 
 @table @option
-@item change @var{diskdevice} @var{filename} [@var{format}]
+@item change @var{diskdevice} @var{filename} [@var{format} [@var{read-only}]]
 Change the medium for a removable disk device to point to @var{filename}. eg
 
 @example
@@ -218,6 +218,20 @@ Change the medium for a removable disk device to point to @var{filename}. eg
 
 @var{format} is optional.
 
+@var{read-only} may be used to change the read-only status of the device. It
+accepts the following values:
+
+@table @var
+@item retain
+Retains the current status; this is the default.
+
+@item ro
+Makes the device read-only.
+
+@item rw
+Makes the device writable.
+@end table
+
 @item change vnc @var{display},@var{options}
 Change the configuration of the VNC server. The valid syntax for @var{display}
 and @var{options} are described at @ref{sec_invocation}. eg
diff --git a/hmp.c b/hmp.c
index 840da5c..f8769b9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -23,6 +23,7 @@
 #include "monitor/monitor.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/string-output-visitor.h"
+#include "qapi/util.h"
 #include "qapi-visit.h"
 #include "ui/console.h"
 #include "block/qapi.h"
@@ -1177,9 +1178,15 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_str(qdict, "device");
     const char *target = qdict_get_str(qdict, "target");
     const char *arg = qdict_get_try_str(qdict, "arg");
+    const char *read_only = qdict_get_try_str(qdict, "read-only");
+    BlockdevChangeReadOnlyMode read_only_mode = 0;
     Error *err = NULL;
 
     if (strcmp(device, "vnc") == 0) {
+        if (read_only) {
+            monitor_printf(mon, "Parameter 'read-only' is invalid for VNC");
+            return;
+        }
         if (strcmp(target, "passwd") == 0 ||
             strcmp(target, "password") == 0) {
             if (!arg) {
@@ -1189,7 +1196,19 @@ void hmp_change(Monitor *mon, const QDict *qdict)
         }
         qmp_change("vnc", target, !!arg, arg, &err);
     } else {
-        qmp_blockdev_change_medium(device, target, !!arg, arg, false, 0, &err);
+        if (read_only) {
+            read_only_mode =
+                qapi_enum_parse(BlockdevChangeReadOnlyMode_lookup,
+                                read_only, BLOCKDEV_CHANGE_READ_ONLY_MODE_MAX,
+                                BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, &err);
+            if (err) {
+                hmp_handle_error(mon, &err);
+                return;
+            }
+        }
+
+        qmp_blockdev_change_medium(device, target, !!arg, arg,
+                                   !!read_only, read_only_mode, &err);
         if (err &&
             error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
             error_free(err);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 4/4] hmp: Add read-only option to change command

[Eric Blake]

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

On 12/05/2014 03:08 AM, Max Reitz wrote:
> Expose the new read-only option of 'blockdev-change-medium' for the
> 'change' HMP command.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  hmp-commands.hx | 20 +++++++++++++++++---
>  hmp.c           | 21 ++++++++++++++++++++-
>  2 files changed, 37 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Eric Blake]

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

On 12/05/2014 03:08 AM, Max Reitz wrote:
> The 'change' QMP and HMP command allows replacing the medium in drives
> which support this, e.g. floppy disk drives. For some drives, the medium
> carries information about whether it can be written to or not (again,
> floppy drives). Therefore, it should be possible to change the read-only
> state of block devices when changing the loaded medium.
> 
> Following a suggestion from Eric, this series first introduces a
> 'blockdev-change-medium' QMP command which is intended to replace the
> 'change' command for block devices. Then, an optional additional
> 'read-only' parameter is added which allows chaning the read-only state

s/chaning/changing/ (I first read it as chaining) - of course, typos in
cover letters don't really matter in the long run :)

> in three ways:
> 
> - 'retain': Just keep the status as it was before; this is the current
>   behavior and thus this will be the default.
> - 'ro': Force read-only access
> - 'rw': Force writable access
> 
> Finally, that 'read-only' parameter is added to the HMP 'change'
> command. This series does not add a 'blockdev-change-medium' QMP command

I assume you meant HMP in this line.

> because 'change' being overloaded for VNC and block devices is not too
> bad for HMP (while it is for QMP).

I agree with that approach.

> 
> 
> v2:
> - basically completely rewritten
> - Dropped 'auto' [Kevin and Markus]
> - Introduced blockdev-change-medium [Eric]
> 
> - Patch 1 introduces the new QMP command 'blockdev-change-medium'; there
>   are (at least) two questionable design choices which I want to explain
>   here:
>   - The name is rather long; furthermore, the name 'change-blockdev' was
>     already suggested by the existing code. I used such a long name
>     because (1) there are no *-blockdev commands, but there are
>     blockdev-* commands, so "blockdev" should be the prefix, not the
>     suffix, and (2) "blockdev-change" could mean anything, so I wanted
>     to be as clear as possible.

That's actually a good explanation; I'm fine with the name you ended up
with, even if it feels long.

>   - The 'format' argument is optional; this is because by making it
>     mandatory, it would have been difficult for the 'change' QMP and HMP
>     commands to retain their 'format' argument optional as well (which
>     we have to do thanks to compatibility)

Yep, I can see that.  On the other hand, the other questionable feature
of 'change' was that it required a filename, but then allowed "" as the
filename that meant no new medium.  In patch 1/4, I wonder if we should
make the new command a bit stricter, by having 'filename' be optional,
and by forbidding "" as a filename (it's still a 1:1 mapping to the old
semantics, and while it would require more HMP glue, it would feel a bit
cleaner from the interface side).

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Max Reitz]

On 2014-12-05 at 14:14, Eric Blake wrote:
> On 12/05/2014 03:08 AM, Max Reitz wrote:
>> The 'change' QMP and HMP command allows replacing the medium in drives
>> which support this, e.g. floppy disk drives. For some drives, the medium
>> carries information about whether it can be written to or not (again,
>> floppy drives). Therefore, it should be possible to change the read-only
>> state of block devices when changing the loaded medium.
>>
>> Following a suggestion from Eric, this series first introduces a
>> 'blockdev-change-medium' QMP command which is intended to replace the
>> 'change' command for block devices. Then, an optional additional
>> 'read-only' parameter is added which allows chaning the read-only state
> s/chaning/changing/ (I first read it as chaining) - of course, typos in
> cover letters don't really matter in the long run :)
>
>> in three ways:
>>
>> - 'retain': Just keep the status as it was before; this is the current
>>    behavior and thus this will be the default.
>> - 'ro': Force read-only access
>> - 'rw': Force writable access
>>
>> Finally, that 'read-only' parameter is added to the HMP 'change'
>> command. This series does not add a 'blockdev-change-medium' QMP command
> I assume you meant HMP in this line.

Yes, right.

>> because 'change' being overloaded for VNC and block devices is not too
>> bad for HMP (while it is for QMP).
> I agree with that approach.
>
>>
>> v2:
>> - basically completely rewritten
>> - Dropped 'auto' [Kevin and Markus]
>> - Introduced blockdev-change-medium [Eric]
>>
>> - Patch 1 introduces the new QMP command 'blockdev-change-medium'; there
>>    are (at least) two questionable design choices which I want to explain
>>    here:
>>    - The name is rather long; furthermore, the name 'change-blockdev' was
>>      already suggested by the existing code. I used such a long name
>>      because (1) there are no *-blockdev commands, but there are
>>      blockdev-* commands, so "blockdev" should be the prefix, not the
>>      suffix, and (2) "blockdev-change" could mean anything, so I wanted
>>      to be as clear as possible.
> That's actually a good explanation; I'm fine with the name you ended up
> with, even if it feels long.
>
>>    - The 'format' argument is optional; this is because by making it
>>      mandatory, it would have been difficult for the 'change' QMP and HMP
>>      commands to retain their 'format' argument optional as well (which
>>      we have to do thanks to compatibility)
> Yep, I can see that.  On the other hand, the other questionable feature
> of 'change' was that it required a filename, but then allowed "" as the
> filename that meant no new medium.

Hm, really?

HMP:

(qemu) change fdd ""
Could not open image: No such file or directory

QMP:

{'execute':'change','arguments':{'device':'fdd','target':''}}
{"timestamp": {"seconds": 1417785755, "microseconds": 519130}, "event": 
"DEVICE_TRAY_MOVED", "data": {"device": "fdd", "tray-open": true}}
{"error": {"class": "GenericError", "desc": "Could not open image: No 
such file or directory"}}

Technically, the medium is now empty, but that wasn't a successful 
execution.

> In patch 1/4, I wonder if we should
> make the new command a bit stricter, by having 'filename' be optional,
> and by forbidding "" as a filename (it's still a 1:1 mapping to the old
> semantics, and while it would require more HMP glue, it would feel a bit
> cleaner from the interface side).

If it was indeed possible somehow, I'd vote for disallowing empty file 
names for blockdev-change-medium and instead invoke 'eject' from the 
'change' commands (and point to 'eject' in the 'change' documentation).

Max

Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> The 'change' QMP and HMP command allows replacing the medium in drives
> which support this, e.g. floppy disk drives. For some drives, the medium
> carries information about whether it can be written to or not (again,
> floppy drives). Therefore, it should be possible to change the read-only
> state of block devices when changing the loaded medium.
>
> Following a suggestion from Eric, this series first introduces a
> 'blockdev-change-medium' QMP command which is intended to replace the
> 'change' command for block devices. Then, an optional additional
> 'read-only' parameter is added which allows chaning the read-only state
> in three ways:
>
> - 'retain': Just keep the status as it was before; this is the current
>   behavior and thus this will be the default.
> - 'ro': Force read-only access
> - 'rw': Force writable access
>
> Finally, that 'read-only' parameter is added to the HMP 'change'
> command. This series does not add a 'blockdev-change-medium' QMP command
> because 'change' being overloaded for VNC and block devices is not too
> bad for HMP (while it is for QMP).

Debatable, but let's hash out QMP before we worry about HMP.

I'd like to make sure that new commands to control removable media get
us closer to a sane set of such commands.  Let's consider states and
transitions.

If we ignore the tray lock for a moment, we have:

                                    load
                    tray open  --------------->  tray closed
                      empty    <---------------    empty
                      ^   |        eject          
                      |   |
        remove medium |   | insert medium
                      |   |
                      |   v        load
                    tray open  --------------->  tray closed
                       full    <---------------    full
                                    eject

Both the operator and the guest OS can load / eject.

Only the operator can remove / insert medium.

A tray lock complicates things a bit.  Each state above splits into a
locked and unlocked state, with the obvious lock / unlock state
transitions.  Only the guest OS can lock / unlock.

When the tray is locked and closed, operator eject merely notifies the
guest OS (blk_dev_eject_request(blk, false)).

In states tray closed / locked, there's an additional operation "eject
forcefully".  It notifies the guest OS (blk_dev_eject_request(blk,
true)), and opens the tray.  Whether unlocks it depends on the device.

Like change, blockdev-change-medium conflates several basic operations.
Is that what we want, or should we create something that lets us do
basic operations?

Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Max Reitz]

On 2014-12-05 at 14:31, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> The 'change' QMP and HMP command allows replacing the medium in drives
>> which support this, e.g. floppy disk drives. For some drives, the medium
>> carries information about whether it can be written to or not (again,
>> floppy drives). Therefore, it should be possible to change the read-only
>> state of block devices when changing the loaded medium.
>>
>> Following a suggestion from Eric, this series first introduces a
>> 'blockdev-change-medium' QMP command which is intended to replace the
>> 'change' command for block devices. Then, an optional additional
>> 'read-only' parameter is added which allows chaning the read-only state
>> in three ways:
>>
>> - 'retain': Just keep the status as it was before; this is the current
>>    behavior and thus this will be the default.
>> - 'ro': Force read-only access
>> - 'rw': Force writable access
>>
>> Finally, that 'read-only' parameter is added to the HMP 'change'
>> command. This series does not add a 'blockdev-change-medium' QMP command
>> because 'change' being overloaded for VNC and block devices is not too
>> bad for HMP (while it is for QMP).
> Debatable, but let's hash out QMP before we worry about HMP.
>
> I'd like to make sure that new commands to control removable media get
> us closer to a sane set of such commands.  Let's consider states and
> transitions.
>
> If we ignore the tray lock for a moment, we have:
>
>                                      load
>                      tray open  --------------->  tray closed
>                        empty    <---------------    empty
>                        ^   |        eject
>                        |   |
>          remove medium |   | insert medium
>                        |   |
>                        |   v        load
>                      tray open  --------------->  tray closed
>                         full    <---------------    full
>                                      eject
>
> Both the operator and the guest OS can load / eject.
>
> Only the operator can remove / insert medium.
>
> A tray lock complicates things a bit.  Each state above splits into a
> locked and unlocked state, with the obvious lock / unlock state
> transitions.  Only the guest OS can lock / unlock.
>
> When the tray is locked and closed, operator eject merely notifies the
> guest OS (blk_dev_eject_request(blk, false)).
>
> In states tray closed / locked, there's an additional operation "eject
> forcefully".  It notifies the guest OS (blk_dev_eject_request(blk,
> true)), and opens the tray.  Whether unlocks it depends on the device.
>
> Like change, blockdev-change-medium conflates several basic operations.
> Is that what we want, or should we create something that lets us do
> basic operations?

Good question. I don't think it will be bad in practice, though. If you 
split up blockdev-change-medium or really only change the medium without 
loading it, then the only advantage that you get is that you can 
exchange media without loading them; I can't really think of a use case 
for that, so in reality you'll always have blockdev-change-medium 
followed immediately by blockdev-load-medium or blockdev-close-tray or 
whatever.

You could split it up even more of course, then you'd have the following 
order for loading a medium:
(1) 'blockdev-open-tray', if not yet open
(2) 'blockdev-remove-medium', if not yet empty
(3) 'blockdev-insert-medium'
(4) 'blockdev-close-tray

I can't think of any time when you'd want to call insert-medium without 
close-tray or without having called remove-medium before (well, if it's 
empty, you don't have to, but well...).

And 'eject' does blockdev-open-tray plus blockdev-remove-medium, so...

Or better, let's collect use cases:
(1) Insert medium into empty drive and load it: Works with 
'blockdev-change-medium'
(2) Open drive, remove medium: Works with 'eject'
(3) Open drive, remove medium, insert medium, close drive: Works with 
'blockdev-change-medium'
(4) Open drive, remove medium, close drive: Does not work with only 
'eject' and 'blockdev-change-medium', but I can't see a difference 
between an open drive and a closed empty drive
(5) Open drive, repeatedly change medium, close drive: Does not work 
with 'blockdev-change-medium' because the guest will see all the media 
you cycled through; but I don't consider this an important use case

So, of course it may be nice in principle to have broken it down to the 
fundamental operations, but I don't see the practical implication.

Hm, well, there is one. I remember someone complaining that 'eject' 
sometimes removes the medium and sometimes doesn't. It did remove the 
medium when qemu could immediately eject it; but it didn't if the drive 
was locked, the guest was notified and then the guest opened the tray. 
So that is a practical implication, because after calling 
blockdev-open-tray, you'd be sure that the medium is still inserted.

I personally don't have a strong opinion. Introducing more commands 
would be work, but I guess I would have time for that now.

Max

Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Eric Blake]

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

On 12/05/2014 06:47 AM, Max Reitz wrote:
>> I'd like to make sure that new commands to control removable media get
>> us closer to a sane set of such commands.  Let's consider states and
>> transitions.
>>
>> If we ignore the tray lock for a moment, we have:
>>
>>                                      load
>>                      tray open  --------------->  tray closed
>>                        empty    <---------------    empty
>>                        ^   |        eject
>>                        |   |
>>          remove medium |   | insert medium
>>                        |   |
>>                        |   v        load
>>                      tray open  --------------->  tray closed
>>                         full    <---------------    full
>>                                      eject

By name, this command feels like it is just the remove/insert medium
step, and unrelated to tray open/close steps.

>>
>> Both the operator and the guest OS can load / eject.
>>
>> Only the operator can remove / insert medium.

Where change is a special case of remove and insert done together.

>>
>> A tray lock complicates things a bit.  Each state above splits into a
>> locked and unlocked state, with the obvious lock / unlock state
>> transitions.  Only the guest OS can lock / unlock.
>>
>> When the tray is locked and closed, operator eject merely notifies the
>> guest OS (blk_dev_eject_request(blk, false)).
>>
>> In states tray closed / locked, there's an additional operation "eject
>> forcefully".  It notifies the guest OS (blk_dev_eject_request(blk,
>> true)), and opens the tray.  Whether unlocks it depends on the device.
>>
>> Like change, blockdev-change-medium conflates several basic operations.
>> Is that what we want, or should we create something that lets us do
>> basic operations?

change lacks a force parameter, which means that it feels like something
that can only be attempted when the tray is open.  The fact that we
(can) make it auto-open an (unlocked) tray under the hood may be okay
for HMP, but should not be part of the QMP command.

> 
> Good question. I don't think it will be bad in practice, though. If you
> split up blockdev-change-medium or really only change the medium without
> loading it, then the only advantage that you get is that you can
> exchange media without loading them; I can't really think of a use case
> for that, so in reality you'll always have blockdev-change-medium
> followed immediately by blockdev-load-medium or blockdev-close-tray or
> whatever.
> 
> You could split it up even more of course, then you'd have the following
> order for loading a medium:
> (1) 'blockdev-open-tray', if not yet open
> (2) 'blockdev-remove-medium', if not yet empty
> (3) 'blockdev-insert-medium'

I'm not sure if 2 and 3 need to be separate, or if they can be a single
change-medium command.

> (4) 'blockdev-close-tray

But yes, I think we need separate QMP commands for tray movement as
compared to medium changes, and that medium change should fail if the
tray is closed.  Higher level software can string together multiple QMP
commands to give the user a single 'change' experience, as desired, but
I don't think QMP should provide such a command, as it is harder to
determine what state things are left in if it fails halfway through.

> 
> I can't think of any time when you'd want to call insert-medium without
> close-tray or without having called remove-medium before (well, if it's
> empty, you don't have to, but well...).
> 
> And 'eject' does blockdev-open-tray plus blockdev-remove-medium, so...
> 
> Or better, let's collect use cases:

Use cases are the HMP (or libvirt) side of things.  If the use case can
be accomplished by stringing together multiple well-defined QMP
operations, instead of having an overloaded QMP command that does
multiple steps, that's still okay for the user's point of view .

> (1) Insert medium into empty drive and load it: Works with
> 'blockdev-change-medium'

Umm, I'm actually agreeing with Markus that we probably want to separate
changing the medium from closing the tray; while for floppies, there is
no tray, so changing a medium is all the more you need to do.

> (2) Open drive, remove medium: Works with 'eject'

What about 'open drive, but leave medium in place for next time drive is
closed'.  Does 'eject' do that (possibly via the addition of an optional
boolean that says whether to leave the medium intact)?  Also, floppy
drives can be locked, but have no tray; do we have the right semantics
for requesting an eject but waiting for the guest to comply by unlocking
the drive?

> (3) Open drive, remove medium, insert medium, close drive: Works with
> 'blockdev-change-medium'

Or rather, works with the high-level HMP 'change', and may be
implemented via multiple QMP commands under the hood.

> (4) Open drive, remove medium, close drive: Does not work with only
> 'eject' and 'blockdev-change-medium', but I can't see a difference
> between an open drive and a closed empty drive

For floppies, there isn't a difference (no tray, so no medium is all the
more distinction you get).  But for cdroms, the guest knows if the tray
is still open or closed (it doesn't know if the medium is present when
the tray is open; the fact that qemu still tracks medium for an open
tray is for convenience in closing the tray and still having the medium
ready to use if it wasn't changed while the tray was open).

> (5) Open drive, repeatedly change medium, close drive: Does not work
> with 'blockdev-change-medium' because the guest will see all the media
> you cycled through; but I don't consider this an important use case

May not be important, and may not be something we expose through HMP,
but I agree with Markus that it would be nice to let the QMP side allow
this.

> 
> So, of course it may be nice in principle to have broken it down to the
> fundamental operations, but I don't see the practical implication.
> 
> Hm, well, there is one. I remember someone complaining that 'eject'
> sometimes removes the medium and sometimes doesn't. It did remove the
> medium when qemu could immediately eject it; but it didn't if the drive
> was locked, the guest was notified and then the guest opened the tray.
> So that is a practical implication, because after calling
> blockdev-open-tray, you'd be sure that the medium is still inserted.
> 
> I personally don't have a strong opinion. Introducing more commands
> would be work, but I guess I would have time for that now.

Requiring multiple QMP commands to do a high-level HMP operation may
feel like overkill, but in the long run the finer granularity will be
worth it.  I think that limiting 'blockdev-change-medium' to work only
on unlocked floppy drives and only on open-tray cdrom drives, plus the
additional glue to open/close the cdrom tray as necessary, will be worth
the effort.

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] blockdev: Add blockdev-change-medium with read-only option

[Max Reitz]

On 2014-12-05 at 15:07, Eric Blake wrote:
> On 12/05/2014 06:47 AM, Max Reitz wrote:
>>> I'd like to make sure that new commands to control removable media get
>>> us closer to a sane set of such commands.  Let's consider states and
>>> transitions.
>>>
>>> If we ignore the tray lock for a moment, we have:
>>>
>>>                                       load
>>>                       tray open  --------------->  tray closed
>>>                         empty    <---------------    empty
>>>                         ^   |        eject
>>>                         |   |
>>>           remove medium |   | insert medium
>>>                         |   |
>>>                         |   v        load
>>>                       tray open  --------------->  tray closed
>>>                          full    <---------------    full
>>>                                       eject
> By name, this command feels like it is just the remove/insert medium
> step, and unrelated to tray open/close steps.
>
>>> Both the operator and the guest OS can load / eject.
>>>
>>> Only the operator can remove / insert medium.
> Where change is a special case of remove and insert done together.
>
>>> A tray lock complicates things a bit.  Each state above splits into a
>>> locked and unlocked state, with the obvious lock / unlock state
>>> transitions.  Only the guest OS can lock / unlock.
>>>
>>> When the tray is locked and closed, operator eject merely notifies the
>>> guest OS (blk_dev_eject_request(blk, false)).
>>>
>>> In states tray closed / locked, there's an additional operation "eject
>>> forcefully".  It notifies the guest OS (blk_dev_eject_request(blk,
>>> true)), and opens the tray.  Whether unlocks it depends on the device.
>>>
>>> Like change, blockdev-change-medium conflates several basic operations.
>>> Is that what we want, or should we create something that lets us do
>>> basic operations?
> change lacks a force parameter, which means that it feels like something
> that can only be attempted when the tray is open.  The fact that we
> (can) make it auto-open an (unlocked) tray under the hood may be okay
> for HMP, but should not be part of the QMP command.
>
>> Good question. I don't think it will be bad in practice, though. If you
>> split up blockdev-change-medium or really only change the medium without
>> loading it, then the only advantage that you get is that you can
>> exchange media without loading them; I can't really think of a use case
>> for that, so in reality you'll always have blockdev-change-medium
>> followed immediately by blockdev-load-medium or blockdev-close-tray or
>> whatever.
>>
>> You could split it up even more of course, then you'd have the following
>> order for loading a medium:
>> (1) 'blockdev-open-tray', if not yet open
>> (2) 'blockdev-remove-medium', if not yet empty
>> (3) 'blockdev-insert-medium'
> I'm not sure if 2 and 3 need to be separate, or if they can be a single
> change-medium command.

Rather not. I want to be able to remove a medium without having to 
insert a new one (you can make the new medium optional for 
change-medium, but I think it's better to just separate the commands).

>> (4) 'blockdev-close-tray
> But yes, I think we need separate QMP commands for tray movement as
> compared to medium changes, and that medium change should fail if the
> tray is closed.  Higher level software can string together multiple QMP
> commands to give the user a single 'change' experience, as desired,

We'll have to keep 'change' anyway, and I wouldn't want to remove it for 
HMP.

> but
> I don't think QMP should provide such a command,

Well, it will, because we will have to keep 'change'.

> as it is harder to
> determine what state things are left in if it fails halfway through.
>
>> I can't think of any time when you'd want to call insert-medium without
>> close-tray or without having called remove-medium before (well, if it's
>> empty, you don't have to, but well...).
>>
>> And 'eject' does blockdev-open-tray plus blockdev-remove-medium, so...
>>
>> Or better, let's collect use cases:
> Use cases are the HMP (or libvirt) side of things.  If the use case can
> be accomplished by stringing together multiple well-defined QMP
> operations, instead of having an overloaded QMP command that does
> multiple steps, that's still okay for the user's point of view .

Right. The question for me was whether there's a practical benefit. If 
you can split an operation into four fundamental steps but nobody would 
ever execute anything different than those four steps in the exactly 
same order, there is no real reason to split.

(Disclaimer: That was just an example which doesn't have to apply here ;-))

>> (1) Insert medium into empty drive and load it: Works with
>> 'blockdev-change-medium'
> Umm, I'm actually agreeing with Markus that we probably want to separate
> changing the medium from closing the tray; while for floppies, there is
> no tray, so changing a medium is all the more you need to do.

For me, floppy trays work like this:
- tray closed, no medium: Doesn't exist. qemu might have that state 
internally, but to the guest it should always look like the tray is open.
- tray open, no medium: Drive is empty.
- tray closed, with medium: Medium is inserted.
- tray open, with medium: Medium was just pushed out or is ready to be 
pushed in (so there is a medium in the slot, but it isn't pushed in). To 
the guest, there is no difference to the "tray open, no medium" state.

So we can indeed just go with the standard four states, only that the 
guest sees only two of them (but that's the problem of qemu's floppy 
emulation).

For the record, I'm looking forward when we're trying to explain floppy 
handling to developers even younger than me in a couple of years. :-)

>> (2) Open drive, remove medium: Works with 'eject'
> What about 'open drive, but leave medium in place for next time drive is
> closed'.  Does 'eject' do that (possibly via the addition of an optional
> boolean that says whether to leave the medium intact)?

Urgh, I'd avoid that. If that's the alternative you're threatening me 
with, I will split up 'eject', fine. :-)

> Also, floppy
> drives can be locked, but have no tray; do we have the right semantics
> for requesting an eject but waiting for the guest to comply by unlocking
> the drive?

I don't know and I'm not sure whether I want to take a look into the 
floppy emulation code. *g*

>> (3) Open drive, remove medium, insert medium, close drive: Works with
>> 'blockdev-change-medium'
> Or rather, works with the high-level HMP 'change', and may be
> implemented via multiple QMP commands under the hood.
>
>> (4) Open drive, remove medium, close drive: Does not work with only
>> 'eject' and 'blockdev-change-medium', but I can't see a difference
>> between an open drive and a closed empty drive
> For floppies, there isn't a difference (no tray, so no medium is all the
> more distinction you get).  But for cdroms, the guest knows if the tray
> is still open or closed

I know, but does the guest care? Or, to be more exact, should it care so 
much that the user actually wants to be able to enforce and empty closed 
drive?

> (it doesn't know if the medium is present when
> the tray is open; the fact that qemu still tracks medium for an open
> tray is for convenience in closing the tray and still having the medium
> ready to use if it wasn't changed while the tray was open).

I wouldn't just call it convenience. qemu is a system emulator, so it 
should emulate a system both to the guest and the user. The user may 
expect to be able to have a medium in an open drive, so that's what qemu 
should support, too.

>> (5) Open drive, repeatedly change medium, close drive: Does not work
>> with 'blockdev-change-medium' because the guest will see all the media
>> you cycled through; but I don't consider this an important use case
> May not be important, and may not be something we expose through HMP,
> but I agree with Markus that it would be nice to let the QMP side allow
> this.
>
>> So, of course it may be nice in principle to have broken it down to the
>> fundamental operations, but I don't see the practical implication.
>>
>> Hm, well, there is one. I remember someone complaining that 'eject'
>> sometimes removes the medium and sometimes doesn't. It did remove the
>> medium when qemu could immediately eject it; but it didn't if the drive
>> was locked, the guest was notified and then the guest opened the tray.
>> So that is a practical implication, because after calling
>> blockdev-open-tray, you'd be sure that the medium is still inserted.
>>
>> I personally don't have a strong opinion. Introducing more commands
>> would be work, but I guess I would have time for that now.
> Requiring multiple QMP commands to do a high-level HMP operation may
> feel like overkill, but in the long run the finer granularity will be
> worth it.  I think that limiting 'blockdev-change-medium' to work only
> on unlocked floppy drives and only on open-tray cdrom drives, plus the
> additional glue to open/close the cdrom tray as necessary, will be worth
> the effort.

At the least it'll be a good example to other QMP commands. *g*

Okay, will rewrite again. v3 might therefore take a bit.

Max