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

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

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

This series adds an optional additional parameter to the 'change' QMP
and HMP command which allows changing the read-only state in four 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
- 'auto': This opens the new file R/W first, if that fails, the file is
  opened read-only.


Max Reitz (3):
  blockdev: Add read-only option to change-blockdev
  qmp: Expose read-only option for 'change'
  hmp: Expose read-only option for 'change'

 blockdev.c                | 41 ++++++++++++++++++++++++++++++++++++++---
 hmp-commands.hx           | 24 +++++++++++++++++++++---
 hmp.c                     | 17 ++++++++++++++++-
 include/sysemu/blockdev.h |  3 ++-
 qapi-schema.json          | 27 ++++++++++++++++++++++++++-
 qmp-commands.hx           | 24 +++++++++++++++++++++++-
 qmp.c                     | 14 ++++++++++++--
 7 files changed, 138 insertions(+), 12 deletions(-)

-- 
1.9.3

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

[Max Reitz]

Add an option to qmp_change_blockdev() which allows changing the
read-only status of the block device to be 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                | 41 ++++++++++++++++++++++++++++++++++++++---
 include/sysemu/blockdev.h |  3 ++-
 qapi-schema.json          | 20 ++++++++++++++++++++
 qmp.c                     |  3 ++-
 4 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a52f205..4140a27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1721,7 +1721,8 @@ 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)
+                         const char *format,
+                         BlockdevChangeReadOnlyMode read_only, Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
@@ -1754,10 +1755,44 @@ void qmp_change_blockdev(const char *device, const char *filename,
         goto out;
     }
 
-    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+    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;
+
+        case BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO:
+            /* try RDWR first */
+            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);
+    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
+
+    if (err) {
+        if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
+            error_free(err);
+            err = NULL;
+
+            /* RDWR did not work, try RO now */
+            bdrv_flags &= ~BDRV_O_RDWR;
+            qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
+        } else {
+            error_propagate(errp, err);
+        }
+    }
 
 out:
     aio_context_release(aio_context);
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 09d1e30..14b4dfb 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -66,7 +66,8 @@ 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);
+                         const char *format,
+                         BlockdevChangeReadOnlyMode read_only, 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 d0926d9..441e001 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1543,6 +1543,26 @@
 { 'command': 'change-vnc-password', 'data': {'password': 'str'} }
 
 ##
+# @BlockdevChangeReadOnlyMode:
+#
+# Specifies the new read-only mode of a block device subject to the @change
+# command.
+#
+# @retain:  Retains the current read-only mode
+#
+# @ro:      Makes the device read-only
+#
+# @rw:      Makes the device writable
+#
+# @auto:    If the file specified can be opened with write access, the block
+#           device will be writable; otherwise it will be read-only
+#
+# Since: 2.3
+##
+{ 'enum': 'BlockdevChangeReadOnlyMode',
+  'data': ['retain', 'ro', 'rw', 'auto'] }
+
+##
 # @change:
 #
 # This command is multiple commands multiplexed together.
diff --git a/qmp.c b/qmp.c
index 0b4f131..bd63cf4 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_change_blockdev(device, target, arg, errp);
+        qmp_change_blockdev(device, target, arg,
+                            BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, errp);
     }
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] qmp: Expose read-only option for 'change'

[Max Reitz]

Expose the new read-only option of qmp_change_blockdev() for the
'change' QMP command. Leave it unset for HMP for now.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hmp.c            |  2 +-
 qapi-schema.json |  7 ++++++-
 qmp-commands.hx  | 24 +++++++++++++++++++++++-
 qmp.c            | 15 ++++++++++++---
 4 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/hmp.c b/hmp.c
index 94b27df..0719fa3 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1133,7 +1133,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
         }
     }
 
-    qmp_change(device, target, !!arg, arg, &err);
+    qmp_change(device, target, !!arg, arg, false, 0, &err);
     if (err &&
         error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
         error_free(err);
diff --git a/qapi-schema.json b/qapi-schema.json
index 441e001..80aaa63 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1581,6 +1581,10 @@
 #          password to set.  If this argument is an empty string, then no future
 #          logins will be allowed.
 #
+# @read-only: #optional, only valid for block devices. This option allows
+#             changing the read-only mode of the block device; defaults to
+#             'retain'. (Since 2.3)
+#
 # Returns: Nothing on success.
 #          If @device is not a valid block device, DeviceNotFound
 #          If the new block device is encrypted, DeviceEncrypted.  Note that
@@ -1595,7 +1599,8 @@
 # Since: 0.14.0
 ##
 { 'command': 'change',
-  'data': {'device': 'str', 'target': 'str', '*arg': 'str'} }
+  'data': {'device': 'str', 'target': 'str', '*arg': 'str',
+           '*read-only': 'BlockdevChangeReadOnlyMode'} }
 
 ##
 # @ObjectTypeInfo:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6ef1b28..edc1783 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -109,7 +109,7 @@ EQMP
 
     {
         .name       = "change",
-        .args_type  = "device:B,target:F,arg:s?",
+        .args_type  = "device:B,target:F,arg:s?,read-only:s?",
         .mhandler.cmd_new = qmp_marshal_input_change,
     },
 
@@ -124,6 +124,8 @@ Arguments:
 - "device": device name (json-string)
 - "target": filename or item (json-string)
 - "arg": additional argument (json-string, optional)
+- "read-only": new read-only mode (json-string, optional)
+          - Possible values: "retain" (default), "ro", "rw", "auto"
 
 Examples:
 
@@ -141,6 +143,26 @@ Examples:
                             "arg": "foobar1" } }
 <- { "return": {} }
 
+3. Load a read-only medium into a writable drive
+
+-> { "execute": "change",
+             "arguments": { "device": "isa-fd0",
+                            "target": "/srv/images/ro.img",
+                            "arg": "raw",
+                            "read-only": "retain" } }
+
+<- { "error":
+     { "class": "GenericError",
+       "desc": "Could not open '/srv/images/ro.img': Permission denied" } }
+
+-> { "execute": "change",
+             "arguments": { "device": "isa-fd0",
+                            "target": "/srv/images/ro.img",
+                            "arg": "raw",
+                            "read-only": "auto" } }
+
+<- { "return": {} }
+
 EQMP
 
     {
diff --git a/qmp.c b/qmp.c
index bd63cf4..d12035b 100644
--- a/qmp.c
+++ b/qmp.c
@@ -397,13 +397,22 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
 #endif /* !CONFIG_VNC */
 
 void qmp_change(const char *device, const char *target,
-                bool has_arg, const char *arg, Error **errp)
+                bool has_arg, const char *arg,
+                bool has_read_only, BlockdevChangeReadOnlyMode read_only,
+                Error **errp)
 {
+    if (!has_read_only) {
+        read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
+    }
+
     if (strcmp(device, "vnc") == 0) {
+        if (has_read_only) {
+            error_setg(errp, "Parameter 'read-only' is invalid for VNC");
+            return;
+        }
         qmp_change_vnc(target, has_arg, arg, errp);
     } else {
-        qmp_change_blockdev(device, target, arg,
-                            BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, errp);
+        qmp_change_blockdev(device, target, arg, read_only, errp);
     }
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 3/3] hmp: Expose read-only option for 'change'

[Max Reitz]

Expose the new read-only option of qmp_change_blockdev() for the
'change' HMP command.

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

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..f0ec0da 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,24 @@ 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.
+
+@item auto
+If @var{filename} can be opened with write access, the block device will be
+writable; otherwise it will be read-only.
+@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 0719fa3..c25e7dc 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"
@@ -1122,6 +1123,8 @@ 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 &&
@@ -1133,7 +1136,19 @@ void hmp_change(Monitor *mon, const QDict *qdict)
         }
     }
 
-    qmp_change(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_change(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 0/3] blockdev: Add read-only option to change-blockdev

[Kevin Wolf]

Am 20.11.2014 um 13:44 hat Max Reitz geschrieben:
> 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.
> 
> This series adds an optional additional parameter to the 'change' QMP
> and HMP command which allows changing the read-only state in four 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
> - 'auto': This opens the new file R/W first, if that fails, the file is
>   opened read-only.

Not sure if 'auto' is worth implementing (it's a typical HMP default
action that no QMP client would use, except that it isn't even the
default for HMP), but the implementation looks correct at least.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Eric Blake]

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

On 11/20/2014 05:44 AM, Max Reitz wrote:
> Add an option to qmp_change_blockdev() which allows changing the
> read-only status of the block device to be 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                | 41 ++++++++++++++++++++++++++++++++++++++---
>  include/sysemu/blockdev.h |  3 ++-
>  qapi-schema.json          | 20 ++++++++++++++++++++
>  qmp.c                     |  3 ++-
>  4 files changed, 62 insertions(+), 5 deletions(-)
> 

>  
> -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> +
> +    if (err) {
> +        if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
> +            error_free(err);
> +            err = NULL;
> +
> +            /* RDWR did not work, try RO now */
> +            bdrv_flags &= ~BDRV_O_RDWR;
> +            qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
> +        } else {
> +            error_propagate(errp, err);
> +        }

Umm, why are you propagating the error here manually, when it was
previously propagated as part of the fall-through into the out: label?
Particularly since the second open_encrypted call still relies on
fallthrough for propagating the error?  I think this should be
simplified to:

if (err && read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
    error_free(err);
    err = NULL;
    /* RDWR did not work, try RO now */
    bdrv_flags &= ~BDRV_O_RDWR;
    qmp_bdrv_open_encrypted(...);
}

Otherwise, looks okay to me.

-- 
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 2/3] qmp: Expose read-only option for 'change'

[Eric Blake]

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

On 11/20/2014 05:44 AM, Max Reitz wrote:
> Expose the new read-only option of qmp_change_blockdev() for the
> 'change' QMP command. Leave it unset for HMP for now.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  hmp.c            |  2 +-
>  qapi-schema.json |  7 ++++++-
>  qmp-commands.hx  | 24 +++++++++++++++++++++++-
>  qmp.c            | 15 ++++++++++++---
>  4 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 94b27df..0719fa3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1133,7 +1133,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>          }
>      }
>  
> -    qmp_change(device, target, !!arg, arg, &err);
> +    qmp_change(device, target, !!arg, arg, false, 0, &err);

Passing raw '0' looks a bit odd for an enum value, but since the
has_read_only parameter is false, I guess it doesn't matter.

>      if (strcmp(device, "vnc") == 0) {
> +        if (has_read_only) {
> +            error_setg(errp, "Parameter 'read-only' is invalid for VNC");
> +            return;
> +        }

The 'change' command is quite ugly.  It would be nice if we could
convert it to an automatic union, where the 'device' parameter is the
discriminated union that determines whether we support or reject the
optional 'read-only' argument - except that it is not a true
discriminator (it is either 'vnc' or a device name, with the further
implication that users CANNOT name their block device 'vnc' and still be
able to use this QMP command).  I wonder if it would be better to leave
the ugly command alone, and instead introduce a new command that isn't
multiplexed.  If someone wants to set a disk's read-only status on
change, then they MUST use the nice new command, and leave the old ugly
command for backward compatibility only with no modifications to make it
even uglier.  Furthermore, adding a new command would let someone name
their block device 'vnc' (not that libvirt has any plans to do so).

At any rate, if we decide to live with extending the existing ugly
'change' command, your patch is correct, so here's a reluctant:

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 3/3] hmp: Expose read-only option for 'change'

[Eric Blake]

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

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

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

Even if you go with my suggestion of a new QMP command in 2/3, I'm just
fine with extending the existing HMP command without adding a new name
(although this patch would then have to be modified to call into the new
QMP command as appropriate).

-- 
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 1/3] blockdev: Add read-only option to change-blockdev

[Max Reitz]

On 2014-11-26 at 17:24, Eric Blake wrote:
> On 11/20/2014 05:44 AM, Max Reitz wrote:
>> Add an option to qmp_change_blockdev() which allows changing the
>> read-only status of the block device to be 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                | 41 ++++++++++++++++++++++++++++++++++++++---
>>   include/sysemu/blockdev.h |  3 ++-
>>   qapi-schema.json          | 20 ++++++++++++++++++++
>>   qmp.c                     |  3 ++-
>>   4 files changed, 62 insertions(+), 5 deletions(-)
>>
>>   
>> -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
>> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
>> +
>> +    if (err) {
>> +        if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
>> +            error_free(err);
>> +            err = NULL;
>> +
>> +            /* RDWR did not work, try RO now */
>> +            bdrv_flags &= ~BDRV_O_RDWR;
>> +            qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
>> +        } else {
>> +            error_propagate(errp, err);
>> +        }
> Umm, why are you propagating the error here manually, when it was
> previously propagated as part of the fall-through into the out: label?

Is it? I don't see any error_propagate() after that 
qmp_bdrv_open_encrypted_call()... And also, that call takes "errp" as a 
parameter, so having error_propagate() afterwards would be kind of strange.

The tree I'm looking at is master, commit 
3ef4ebcc5c0360629bb05f49d9b961774247d6ca. In my block-next tree, which 
this patch is actually based against, commit 
656ddfaafd67af2b9234567e51cd3418588b2ddd.

> Particularly since the second open_encrypted call still relies on
> fallthrough for propagating the error?  I think this should be
> simplified to:
>
> if (err && read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
>      error_free(err);
>      err = NULL;
>      /* RDWR did not work, try RO now */
>      bdrv_flags &= ~BDRV_O_RDWR;
>      qmp_bdrv_open_encrypted(...);
> }

But then you're not propagating the error anymore (in case of !err || 
read_only != BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO). As I said, at least 
in my tree, there is no error_propagate(errp, err); after this block. I 
may add it, though.

(to be completely sure: 
http://git.qemu.org/?p=qemu.git;a=blob;f=blockdev.c;h=57910b82c7adc3ce59173afeeebcd37ff2a3dfd0;hb=3ef4ebcc5c0360629bb05f49d9b961774247d6ca#l1727 
and https://github.com/XanClic/qemu/blob/block-next/blockdev.c#L1760)

Max

> Otherwise, looks okay to me.

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

[Eric Blake]

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

On 11/26/2014 09:36 AM, Max Reitz wrote:

>>>   -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL,
>>> errp);
>>> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
>>> +
>>> +    if (err) {
>>> +        if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
>>> +            error_free(err);
>>> +            err = NULL;
>>> +
>>> +            /* RDWR did not work, try RO now */
>>> +            bdrv_flags &= ~BDRV_O_RDWR;
>>> +            qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv,
>>> NULL, errp);
>>> +        } else {
>>> +            error_propagate(errp, err);
>>> +        }
>> Umm, why are you propagating the error here manually, when it was
>> previously propagated as part of the fall-through into the out: label?
> 
> Is it? I don't see any error_propagate() after that
> qmp_bdrv_open_encrypted_call()... And also, that call takes "errp" as a
> parameter, so having error_propagate() afterwards would be kind of strange.

Oh, I read too fast; I'm missing the difference between '&err' and
'errp'.  I think we generally use the name 'local_err' instead of plain
'err', so that it is more obvious when we are collecting into
'&local_err' with a need to propagate, vs. reusing the caller's 'errp'
directly because we aren't further checking things.

Okay, your code is correct after all.  The pre-existing confusion of
'err' instead of 'local_err' might be worth fixing if you have a reason
for a respin, but is not itself a reason for me to withhold approval.

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 0/3] blockdev: Add read-only option to change-blockdev

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 20.11.2014 um 13:44 hat Max Reitz geschrieben:
>> 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.
>> 
>> This series adds an optional additional parameter to the 'change' QMP
>> and HMP command which allows changing the read-only state in four 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
>> - 'auto': This opens the new file R/W first, if that fails, the file is
>>   opened read-only.
>
> Not sure if 'auto' is worth implementing (it's a typical HMP default
> action that no QMP client would use, except that it isn't even the
> default for HMP), but the implementation looks correct at least.

QMP eschews magic.  I'd prefer to keep 'auto' out.

HMP can offer it regardless, if it's useful.  But I doubt it will be.
Few users will need to control this, and fewer will realize they can by
giving an extra argument.

I wish I could tell you to leave change alone because it's a legacy
dungpile.  Alas, it's still only a dungpile.  I hope we can create a set
of sane media control commands relatively soon now.

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

[Max Reitz]

On 2014-11-28 at 16:43, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 20.11.2014 um 13:44 hat Max Reitz geschrieben:
>>> 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.
>>>
>>> This series adds an optional additional parameter to the 'change' QMP
>>> and HMP command which allows changing the read-only state in four 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
>>> - 'auto': This opens the new file R/W first, if that fails, the file is
>>>    opened read-only.
>> Not sure if 'auto' is worth implementing (it's a typical HMP default
>> action that no QMP client would use, except that it isn't even the
>> default for HMP), but the implementation looks correct at least.
> QMP eschews magic.  I'd prefer to keep 'auto' out.
>
> HMP can offer it regardless, if it's useful.  But I doubt it will be.
> Few users will need to control this, and fewer will realize they can by
> giving an extra argument.

Well, Kevin made the good point of the user generally knowing whether a 
file should be written to or not. Furthermore, I don't think it would be 
too hard to use rw, then see that access was denied, and then use ro, 
manually. Maybe that's even better than auto, somehow.

Max

> I wish I could tell you to leave change alone because it's a legacy
> dungpile.  Alas, it's still only a dungpile.  I hope we can create a set
> of sane media control commands relatively soon now.

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2014-11-28 at 16:43, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 20.11.2014 um 13:44 hat Max Reitz geschrieben:
>>>> 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.
>>>>
>>>> This series adds an optional additional parameter to the 'change' QMP
>>>> and HMP command which allows changing the read-only state in four 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
>>>> - 'auto': This opens the new file R/W first, if that fails, the file is
>>>>    opened read-only.
>>> Not sure if 'auto' is worth implementing (it's a typical HMP default
>>> action that no QMP client would use, except that it isn't even the
>>> default for HMP), but the implementation looks correct at least.
>> QMP eschews magic.  I'd prefer to keep 'auto' out.
>>
>> HMP can offer it regardless, if it's useful.  But I doubt it will be.
>> Few users will need to control this, and fewer will realize they can by
>> giving an extra argument.
>
> Well, Kevin made the good point of the user generally knowing whether
> a file should be written to or not. Furthermore, I don't think it
> would be too hard to use rw, then see that access was denied, and then
> use ro, manually. Maybe that's even better than auto, somehow.

Please give it a try.