All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option
@ 2018-10-19 16:30 Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 01/11] block: Update flags in bdrv_set_read_only() Kevin Wolf
                   ` (11 more replies)
  0 siblings, 12 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

See patch 2 for an explanation of the motivation.

v4:
- Split fix for missing rbd_close() into a separate patch [Eric]
- Added qemu-iotests case

v3:
- Clarified QAPI schema documentation that auto-read-only can only
  degrade read-write to read-only, not the other way round [Eric]
- Don't refuse to set copy-on-read=on and auto-read-only=on at the same
  time; only complain when actually trying to degrade to read-only
- Let bdrv_apply_auto_read_only() return -EACCESS on all errors
- Fixed file-posix and gluster implementations [Eric, Niels]
- Added a patch to make auto-read-only=on the default for human user
  interfaces (-drive/-hda/...)

v2:
- Turn bdrv_set_read_only() into bdrv_apply_auto_read_only()
- Support the option in a lot more block drivers

Kevin Wolf (11):
  block: Update flags in bdrv_set_read_only()
  block: Add auto-read-only option
  rbd: Close image in qemu_rbd_open() error path
  block: Require auto-read-only for existing fallbacks
  nbd: Support auto-read-only option
  file-posix: Support auto-read-only option
  curl: Support auto-read-only option
  gluster: Support auto-read-only option
  iscsi: Support auto-read-only option
  block: Make auto-read-only=on default for -drive
  qemu-iotests: Test auto-read-only with -drive and -blockdev

 qapi/block-core.json       |   7 ++
 include/block/block.h      |   5 +-
 block.c                    |  54 +++++++++++---
 block/bochs.c              |  17 ++---
 block/cloop.c              |  16 ++--
 block/curl.c               |   8 +-
 block/dmg.c                |  16 ++--
 block/file-posix.c         |  19 ++++-
 block/gluster.c            |  12 ++-
 block/iscsi.c              |   8 +-
 block/nbd-client.c         |  10 +--
 block/rbd.c                |  14 +---
 block/vvfat.c              |  11 +--
 blockdev.c                 |   1 +
 tests/qemu-iotests/232     | 147 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/232.out |  59 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 17 files changed, 327 insertions(+), 78 deletions(-)
 create mode 100755 tests/qemu-iotests/232
 create mode 100644 tests/qemu-iotests/232.out

-- 
2.19.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 01/11] block: Update flags in bdrv_set_read_only()
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 02/11] block: Add auto-read-only option Kevin Wolf
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

To fully change the read-only state of a node, we must not only change
bs->read_only, but also update bs->open_flags.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 0d6e5f1a76..d7bd6d29b4 100644
--- a/block.c
+++ b/block.c
@@ -281,6 +281,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
     }
 
     bs->read_only = read_only;
+
+    if (read_only) {
+        bs->open_flags &= ~BDRV_O_RDWR;
+    } else {
+        bs->open_flags |= BDRV_O_RDWR;
+    }
+
     return 0;
 }
 
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 02/11] block: Add auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 01/11] block: Update flags in bdrv_set_read_only() Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-31 17:14   ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path Kevin Wolf
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.

Backing files should work on read-only storage, but at the same time, a
block job like commit should be able to reopen them read-write if they
are on read-write storage. However, without option inheritance, reopen
only changes the read-only option for the root node (typically the
format layer), but not the protocol layer, so reopening fails (the
format layer wants to get write permissions, but the protocol layer is
still read-only).

A simple workaround for the problem in the management tool would be to
open the protocol layer always read-write and to make only the format
layer read-only for backing files. However, sometimes the file is
actually stored on read-only storage and we don't know whether the image
can be opened read-write (for example, for NBD it depends on the server
we're trying to connect to). This adds an option that makes QEMU try to
open the image read-write, but allows it to degrade to a read-only mode
without returning an error.

The documentation for this option is consciously phrased in a way that
allows QEMU to switch to a better model eventually: Instead of trying
when the image is first opened, making the read-only flag dynamic and
changing it automatically whenever the first BLK_PERM_WRITE user is
attached or the last one is detached would be much more useful
behaviour.

Unfortunately, this more useful behaviour is also a lot harder to
implement, and libvirt needs a solution now before it can switch to
-blockdev, so let's start with this easier approach for now.

Instead of adding a new auto-read-only option, turning the existing
read-only into an enum (with a bool alternate for compatibility) was
considered, but it complicated the implementation to the point that it
didn't seem to be worth it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-core.json  |  7 +++++++
 include/block/block.h |  2 ++
 block.c               | 17 +++++++++++++++++
 block/vvfat.c         |  1 +
 4 files changed, 27 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index cfb37f8c1d..2ac4e69e68 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3651,6 +3651,12 @@
 #                 either generally or in certain configurations. In this case,
 #                 the default value does not work and the option must be
 #                 specified explicitly.
+# @auto-read-only: if true and @read-only is false, QEMU may automatically
+#                  decide not to open the image read-write as requested, but
+#                  fall back to read-only instead (and switch between the modes
+#                  later), e.g. depending on whether the image file is writable
+#                  or whether a writing user is attached to the node
+#                  (default: false, since 3.1)
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 # @force-share:   force share all permission on added nodes.
@@ -3666,6 +3672,7 @@
             '*discard': 'BlockdevDiscardOptions',
             '*cache': 'BlockdevCacheOptions',
             '*read-only': 'bool',
+            '*auto-read-only': 'bool',
             '*force-share': 'bool',
             '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..580b3716c3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -115,6 +115,7 @@ typedef struct HDGeometry {
                                       select an appropriate protocol driver,
                                       ignoring the format layer */
 #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
+#define BDRV_O_AUTO_RDONLY 0x20000 /* degrade to read-only if opening read-write fails */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
@@ -125,6 +126,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_DIRECT   "cache.direct"
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY      "read-only"
+#define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
 #define BDRV_OPT_DISCARD        "discard"
 #define BDRV_OPT_FORCE_SHARE    "force-share"
 
diff --git a/block.c b/block.c
index d7bd6d29b4..ed73522cc6 100644
--- a/block.c
+++ b/block.c
@@ -930,6 +930,7 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
 
     /* Inherit the read-only option from the parent if it's not set */
     qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_AUTO_READ_ONLY);
 
     /* Our block drivers take care to send flushes and respect unmap policy,
      * so we can default to enable both on lower layers regardless of the
@@ -1053,6 +1054,7 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
 
     /* backing files always opened read-only */
     qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
+    qdict_set_default_str(child_options, BDRV_OPT_AUTO_READ_ONLY, "off");
     flags &= ~BDRV_O_COPY_ON_READ;
 
     /* snapshot=on is handled on the top layer */
@@ -1142,6 +1144,10 @@ static void update_flags_from_options(int *flags, QemuOpts *opts)
         *flags |= BDRV_O_RDWR;
     }
 
+    assert(qemu_opt_find(opts, BDRV_OPT_AUTO_READ_ONLY));
+    if (qemu_opt_get_bool_del(opts, BDRV_OPT_AUTO_READ_ONLY, false)) {
+        *flags |= BDRV_O_AUTO_RDONLY;
+    }
 }
 
 static void update_options_from_flags(QDict *options, int flags)
@@ -1156,6 +1162,10 @@ static void update_options_from_flags(QDict *options, int flags)
     if (!qdict_haskey(options, BDRV_OPT_READ_ONLY)) {
         qdict_put_bool(options, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR));
     }
+    if (!qdict_haskey(options, BDRV_OPT_AUTO_READ_ONLY)) {
+        qdict_put_bool(options, BDRV_OPT_AUTO_READ_ONLY,
+                       flags & BDRV_O_AUTO_RDONLY);
+    }
 }
 
 static void bdrv_assign_node_name(BlockDriverState *bs,
@@ -1328,6 +1338,11 @@ QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Node is opened in read-only mode",
         },
+        {
+            .name = BDRV_OPT_AUTO_READ_ONLY,
+            .type = QEMU_OPT_BOOL,
+            .help = "Node can become read-only if opening read-write fails",
+        },
         {
             .name = "detect-zeroes",
             .type = QEMU_OPT_STRING,
@@ -2486,6 +2501,8 @@ BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
         qdict_set_default_str(qdict, BDRV_OPT_CACHE_DIRECT, "off");
         qdict_set_default_str(qdict, BDRV_OPT_CACHE_NO_FLUSH, "off");
         qdict_set_default_str(qdict, BDRV_OPT_READ_ONLY, "off");
+        qdict_set_default_str(qdict, BDRV_OPT_AUTO_READ_ONLY, "off");
+
     }
 
     bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
diff --git a/block/vvfat.c b/block/vvfat.c
index f2e7d501cf..98ba5e2bac 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3130,6 +3130,7 @@ static void vvfat_qcow_options(int *child_flags, QDict *child_options,
                                int parent_flags, QDict *parent_options)
 {
     qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "off");
+    qdict_set_default_str(child_options, BDRV_OPT_AUTO_READ_ONLY, "off");
     qdict_set_default_str(child_options, BDRV_OPT_CACHE_NO_FLUSH, "on");
 }
 
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 01/11] block: Update flags in bdrv_set_read_only() Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 02/11] block: Add auto-read-only option Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 18:14   ` Eric Blake
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 04/11] block: Require auto-read-only for existing fallbacks Kevin Wolf
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

Commit e2b8247a322 introduced an error path in qemu_rbd_open() after
calling rbd_open(), but neglected to close the image again in this error
path. The error path should contain everything that the regular close
function qemu_rbd_close() contains.

This adds the missing rbd_close() call.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/rbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/rbd.c b/block/rbd.c
index 014c68d629..27c9a1e81c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -787,6 +787,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
                          "automatically marking the image read-only.");
             r = bdrv_set_read_only(bs, true, &local_err);
             if (r < 0) {
+                rbd_close(s->image);
                 error_propagate(errp, local_err);
                 goto failed_open;
             }
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 04/11] block: Require auto-read-only for existing fallbacks
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (2 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 05/11] nbd: Support auto-read-only option Kevin Wolf
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

Some block drivers have traditionally changed their node to read-only
mode without asking the user. This behaviour has been marked deprecated
since 2.11, expecting users to provide an explicit read-only=on option.

Now that we have auto-read-only=on, enable these drivers to make use of
the option.

This is the only use of bdrv_set_read_only(), so we can make it a bit
more specific and turn it into a bdrv_apply_auto_read_only() that is
more convenient for drivers to use.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/block/block.h |  3 ++-
 block.c               | 42 +++++++++++++++++++++++++++---------------
 block/bochs.c         | 17 ++++++-----------
 block/cloop.c         | 16 +++++-----------
 block/dmg.c           | 16 +++++-----------
 block/rbd.c           | 15 ++++-----------
 block/vvfat.c         | 10 ++--------
 7 files changed, 51 insertions(+), 68 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 580b3716c3..7f5453b45b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -438,7 +438,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
 bool bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
                            bool ignore_allow_rdw, Error **errp);
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp);
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+                              Error **errp);
 bool bdrv_is_writable(BlockDriverState *bs);
 bool bdrv_is_sg(BlockDriverState *bs);
 bool bdrv_is_inserted(BlockDriverState *bs);
diff --git a/block.c b/block.c
index ed73522cc6..a97e324f39 100644
--- a/block.c
+++ b/block.c
@@ -266,29 +266,41 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only,
     return 0;
 }
 
-/* TODO Remove (deprecated since 2.11)
- * Block drivers are not supposed to automatically change bs->read_only.
- * Instead, they should just check whether they can provide what the user
- * explicitly requested and error out if read-write is requested, but they can
- * only provide read-only access. */
-int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
+/*
+ * Called by a driver that can only provide a read-only image.
+ *
+ * Returns 0 if the node is already read-only or it could switch the node to
+ * read-only because BDRV_O_AUTO_RDONLY is set.
+ *
+ * Returns -EACCES if the node is read-write and BDRV_O_AUTO_RDONLY is not set
+ * or bdrv_can_set_read_only() forbids making the node read-only. If @errmsg
+ * is not NULL, it is used as the error message for the Error object.
+ */
+int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg,
+                              Error **errp)
 {
     int ret = 0;
 
-    ret = bdrv_can_set_read_only(bs, read_only, false, errp);
-    if (ret < 0) {
-        return ret;
+    if (!(bs->open_flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
+        goto fail;
     }
 
-    bs->read_only = read_only;
-
-    if (read_only) {
-        bs->open_flags &= ~BDRV_O_RDWR;
-    } else {
-        bs->open_flags |= BDRV_O_RDWR;
+    ret = bdrv_can_set_read_only(bs, true, false, NULL);
+    if (ret < 0) {
+        goto fail;
     }
 
+    bs->read_only = true;
+    bs->open_flags &= ~BDRV_O_RDWR;
+
     return 0;
+
+fail:
+    error_setg(errp, "%s", errmsg ?: "Image is read-only");
+    return -EACCES;
 }
 
 void bdrv_get_full_backing_filename_from_filename(const char *backed,
diff --git a/block/bochs.c b/block/bochs.c
index 50c630047b..22e7d44211 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,23 +105,18 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
     struct bochs_header bochs;
     int ret;
 
+    /* No write support yet */
+    ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    if (!bdrv_is_read_only(bs)) {
-        error_report("Opening bochs images without an explicit read-only=on "
-                     "option is deprecated. Future versions will refuse to "
-                     "open the image instead of automatically marking the "
-                     "image read-only.");
-        ret = bdrv_set_read_only(bs, true, errp); /* no write support yet */
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     ret = bdrv_pread(bs->file, 0, &bochs, sizeof(bochs));
     if (ret < 0) {
         return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 2be68987bd..df2b85f723 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,23 +67,17 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
     uint32_t offsets_size, max_compressed_block_size = 1, i;
     int ret;
 
+    ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    if (!bdrv_is_read_only(bs)) {
-        error_report("Opening cloop images without an explicit read-only=on "
-                     "option is deprecated. Future versions will refuse to "
-                     "open the image instead of automatically marking the "
-                     "image read-only.");
-        ret = bdrv_set_read_only(bs, true, errp);
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     /* read header */
     ret = bdrv_pread(bs->file, 128, &s->block_size, 4);
     if (ret < 0) {
diff --git a/block/dmg.c b/block/dmg.c
index c9b3c519c4..1d9283ba2f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -413,23 +413,17 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     int64_t offset;
     int ret;
 
+    ret = bdrv_apply_auto_read_only(bs, NULL, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
     if (!bs->file) {
         return -EINVAL;
     }
 
-    if (!bdrv_is_read_only(bs)) {
-        error_report("Opening dmg images without an explicit read-only=on "
-                     "option is deprecated. Future versions will refuse to "
-                     "open the image instead of automatically marking the "
-                     "image read-only.");
-        ret = bdrv_set_read_only(bs, true, errp);
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     block_module_load_one("dmg-bz2");
 
     s->n_chunks = 0;
diff --git a/block/rbd.c b/block/rbd.c
index 27c9a1e81c..ee0b4a6941 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -780,17 +780,10 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     /* If we are using an rbd snapshot, we must be r/o, otherwise
      * leave as-is */
     if (s->snap != NULL) {
-        if (!bdrv_is_read_only(bs)) {
-            error_report("Opening rbd snapshots without an explicit "
-                         "read-only=on option is deprecated. Future versions "
-                         "will refuse to open the image instead of "
-                         "automatically marking the image read-only.");
-            r = bdrv_set_read_only(bs, true, &local_err);
-            if (r < 0) {
-                rbd_close(s->image);
-                error_propagate(errp, local_err);
-                goto failed_open;
-            }
+        r = bdrv_apply_auto_read_only(bs, "rbd snapshots are read-only", errp);
+        if (r < 0) {
+            rbd_close(s->image);
+            goto failed_open;
         }
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index 98ba5e2bac..e4df255d58 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1262,15 +1262,9 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                        "Unable to set VVFAT to 'rw' when drive is read-only");
             goto fail;
         }
-    } else  if (!bdrv_is_read_only(bs)) {
-        error_report("Opening non-rw vvfat images without an explicit "
-                     "read-only=on option is deprecated. Future versions "
-                     "will refuse to open the image instead of "
-                     "automatically marking the image read-only.");
-        /* read only is the default for safety */
-        ret = bdrv_set_read_only(bs, true, &local_err);
+    } else {
+        ret = bdrv_apply_auto_read_only(bs, NULL, errp);
         if (ret < 0) {
-            error_propagate(errp, local_err);
             goto fail;
         }
     }
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 05/11] nbd: Support auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (3 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 04/11] block: Require auto-read-only for existing fallbacks Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 06/11] file-posix: " Kevin Wolf
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

If read-only=off, but auto-read-only=on is given, open a read-write NBD
connection if the server provides a read-write export, but instead of
erroring out for read-only exports, just degrade to read-only.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 9686ecbd5e..76e9ca3abe 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -992,11 +992,11 @@ int nbd_client_init(BlockDriverState *bs,
         logout("Failed to negotiate with the NBD server\n");
         return ret;
     }
-    if (client->info.flags & NBD_FLAG_READ_ONLY &&
-        !bdrv_is_read_only(bs)) {
-        error_setg(errp,
-                   "request for write access conflicts with read-only export");
-        return -EACCES;
+    if (client->info.flags & NBD_FLAG_READ_ONLY) {
+        ret = bdrv_apply_auto_read_only(bs, "NBD export is read-only", errp);
+        if (ret < 0) {
+            return ret;
+        }
     }
     if (client->info.flags & NBD_FLAG_SEND_FUA) {
         bs->supported_write_flags = BDRV_REQ_FUA;
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 06/11] file-posix: Support auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (4 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 05/11] nbd: Support auto-read-only option Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 07/11] curl: " Kevin Wolf
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/file-posix.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 2da3a76355..0c1b81ce4b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -527,9 +527,22 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     s->fd = -1;
     fd = qemu_open(filename, s->open_flags, 0644);
-    if (fd < 0) {
-        ret = -errno;
-        error_setg_errno(errp, errno, "Could not open '%s'", filename);
+    ret = fd < 0 ? -errno : 0;
+
+    if (ret == -EACCES || ret == -EROFS) {
+        /* Try to degrade to read-only, but if it doesn't work, still use the
+         * normal error message. */
+        if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+            bdrv_flags &= ~BDRV_O_RDWR;
+            raw_parse_flags(bdrv_flags, &s->open_flags);
+            assert(!(s->open_flags & O_CREAT));
+            fd = qemu_open(filename, s->open_flags);
+            ret = fd < 0 ? -errno : 0;
+        }
+    }
+
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not open '%s'", filename);
         if (ret == -EROFS) {
             ret = -EACCES;
         }
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 07/11] curl: Support auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (5 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 06/11] file-posix: " Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 08/11] gluster: " Kevin Wolf
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

If read-only=off, but auto-read-only=on is given, just degrade to
read-only.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/curl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index fabb2b4da7..db5d2bd8ef 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -684,10 +684,10 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
     const char *protocol_delimiter;
     int ret;
 
-
-    if (flags & BDRV_O_RDWR) {
-        error_setg(errp, "curl block device does not support writes");
-        return -EROFS;
+    ret = bdrv_apply_auto_read_only(bs, "curl driver does not support writes",
+                                    errp);
+    if (ret < 0) {
+        return ret;
     }
 
     if (!libcurl_initialized) {
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 08/11] gluster: Support auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (6 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 07/11] curl: " Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 09/11] iscsi: " Kevin Wolf
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

If read-only=off, but auto-read-only=on is given, open the file
read-write if we have the permissions, but instead of erroring out for
read-only files, just degrade to read-only.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Niels de Vos <ndevos@redhat.com>
---
 block/gluster.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..5e300c96c8 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -849,8 +849,16 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
     qemu_gluster_parse_flags(bdrv_flags, &open_flags);
 
     s->fd = glfs_open(s->glfs, gconf->path, open_flags);
-    if (!s->fd) {
-        ret = -errno;
+    ret = s->fd ? 0 : -errno;
+
+    if (ret == -EACCES || ret == -EROFS) {
+        /* Try to degrade to read-only, but if it doesn't work, still use the
+         * normal error message. */
+        if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
+            open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
+            s->fd = glfs_open(s->glfs, gconf->path, open_flags);
+            ret = s->fd ? 0 : -errno;
+        }
     }
 
     s->supports_seek_data = qemu_gluster_test_seek(s->fd);
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 09/11] iscsi: Support auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (7 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 08/11] gluster: " Kevin Wolf
@ 2018-10-19 16:30 ` Kevin Wolf
  2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 10/11] block: Make auto-read-only=on default for -drive Kevin Wolf
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-19 16:30 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

If read-only=off, but auto-read-only=on is given, open the volume
read-write if we have the permissions, but instead of erroring out for
read-only volumes, just degrade to read-only.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/iscsi.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index bb69faf34a..130ae26f5f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1878,9 +1878,11 @@ static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
     /* Check the write protect flag of the LUN if we want to write */
     if (iscsilun->type == TYPE_DISK && (flags & BDRV_O_RDWR) &&
         iscsilun->write_protected) {
-        error_setg(errp, "Cannot open a write protected LUN as read-write");
-        ret = -EACCES;
-        goto out;
+        ret = bdrv_apply_auto_read_only(bs, "LUN is write protected", errp);
+        if (ret < 0) {
+            goto out;
+        }
+        flags &= ~BDRV_O_RDWR;
     }
 
     iscsi_readcapacity_sync(iscsilun, &local_err);
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path Kevin Wolf
@ 2018-10-19 18:14   ` Eric Blake
  0 siblings, 0 replies; 16+ messages in thread
From: Eric Blake @ 2018-10-19 18:14 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: mreitz, pkrempa, qemu-devel

On 10/19/18 11:30 AM, Kevin Wolf wrote:
> Commit e2b8247a322 introduced an error path in qemu_rbd_open() after
> calling rbd_open(), but neglected to close the image again in this error
> path. The error path should contain everything that the regular close
> function qemu_rbd_close() contains.
> 
> This adds the missing rbd_close() call.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/rbd.c | 1 +
>   1 file changed, 1 insertion(+)
> 

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

> diff --git a/block/rbd.c b/block/rbd.c
> index 014c68d629..27c9a1e81c 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -787,6 +787,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>                            "automatically marking the image read-only.");
>               r = bdrv_set_read_only(bs, true, &local_err);
>               if (r < 0) {
> +                rbd_close(s->image);
>                   error_propagate(errp, local_err);
>                   goto failed_open;
>               }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 10/11] block: Make auto-read-only=on default for -drive
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (8 preceding siblings ...)
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 09/11] iscsi: " Kevin Wolf
@ 2018-10-20 19:31 ` Kevin Wolf
  2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 11/11] qemu-iotests: Test auto-read-only with -drive and -blockdev Kevin Wolf
  2018-10-31 17:16 ` [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-20 19:31 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

While we want machine interfaces like -blockdev and QMP blockdev-add to
add as little auto-detection as possible so that management tools are
explicit about their needs, -drive is a convenience option for human
users. Enabling auto-read-only=on by default there enables users to use
read-only images for read-only guest devices without having to specify
read-only=on explicitly. If they try to attach the image to a read-write
device, they will still get an error message.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/blockdev.c b/blockdev.c
index a8755bd908..ab57a8a53a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -590,6 +590,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
         qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
                               read_only ? "on" : "off");
+        qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on");
         assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
         if (runstate_check(RUN_STATE_INMIGRATE)) {
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Qemu-devel] [PATCH v4 11/11] qemu-iotests: Test auto-read-only with -drive and -blockdev
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (9 preceding siblings ...)
  2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 10/11] block: Make auto-read-only=on default for -drive Kevin Wolf
@ 2018-10-20 19:31 ` Kevin Wolf
  2018-10-22 12:14   ` Eric Blake
  2018-10-31 17:16 ` [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
  11 siblings, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2018-10-20 19:31 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, pkrempa, qemu-devel

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/232     | 147 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/232.out |  59 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 207 insertions(+)
 create mode 100755 tests/qemu-iotests/232
 create mode 100644 tests/qemu-iotests/232.out

diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232
new file mode 100755
index 0000000000..bc2972d124
--- /dev/null
+++ b/tests/qemu-iotests/232
@@ -0,0 +1,147 @@
+#!/bin/bash
+#
+# Test for auto-read-only
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f $TEST_IMG.snap
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    (
+        if ! test -t 0; then
+            while read cmd; do
+                echo $cmd
+            done
+        fi
+        echo quit
+    ) | $QEMU -nographic -monitor stdio -nodefaults "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp |
+        _filter_generated_node_ids | _filter_imgfmt
+}
+
+function run_qemu_info_block()
+{
+    echo "info block -n" | run_qemu "$@" | grep -e "(file" -e "QEMU_PROG"
+}
+
+size=128M
+
+_make_test_img $size
+
+echo
+echo "=== -drive with read-write image: read-only/auto-read-only combinations ==="
+echo
+
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=off
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on
+echo
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=off
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off
+echo
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=off
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none
+
+echo
+echo "=== -drive with read-only image: read-only/auto-read-only combinations ==="
+echo
+
+chmod a-w $TEST_IMG
+
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=off
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=on
+echo
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=off
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,read-only=off
+echo
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=off
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none,auto-read-only=on
+run_qemu_info_block -drive driver=file,file="$TEST_IMG",if=none
+
+echo
+echo "=== -blockdev with read-write image: read-only/auto-read-only combinations ==="
+echo
+
+chmod a+w $TEST_IMG
+
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=off
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=on
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on
+echo
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=off
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=on
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off
+echo
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=off
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=on
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0
+
+echo
+echo "=== -blockdev with read-only image: read-only/auto-read-only combinations ==="
+echo
+
+chmod a-w $TEST_IMG
+
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=off
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on,auto-read-only=on
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=on
+echo
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=off
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off,auto-read-only=on
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,read-only=off
+echo
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=off
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=on
+run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/232.out b/tests/qemu-iotests/232.out
new file mode 100644
index 0000000000..dcb683afa3
--- /dev/null
+++ b/tests/qemu-iotests/232.out
@@ -0,0 +1,59 @@
+QA output created by 232
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+
+=== -drive with read-write image: read-only/auto-read-only combinations ===
+
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+
+=== -drive with read-only image: read-only/auto-read-only combinations ===
+
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+
+QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+
+QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+
+=== -blockdev with read-write image: read-only/auto-read-only combinations ===
+
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+
+node0: TEST_DIR/t.IMGFMT (file)
+node0: TEST_DIR/t.IMGFMT (file)
+node0: TEST_DIR/t.IMGFMT (file)
+
+node0: TEST_DIR/t.IMGFMT (file)
+node0: TEST_DIR/t.IMGFMT (file)
+node0: TEST_DIR/t.IMGFMT (file)
+
+=== -blockdev with read-only image: read-only/auto-read-only combinations ===
+
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+node0: TEST_DIR/t.IMGFMT (file, read-only)
+QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 31f6e77dcb..ebe4fe78bc 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -227,3 +227,4 @@
 227 auto quick
 229 auto quick
 231 auto quick
+232 auto quick
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 11/11] qemu-iotests: Test auto-read-only with -drive and -blockdev
  2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 11/11] qemu-iotests: Test auto-read-only with -drive and -blockdev Kevin Wolf
@ 2018-10-22 12:14   ` Eric Blake
  0 siblings, 0 replies; 16+ messages in thread
From: Eric Blake @ 2018-10-22 12:14 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: mreitz, pkrempa, qemu-devel

On 10/20/18 8:31 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/232     | 147 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/232.out |  59 +++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 207 insertions(+)
>   create mode 100755 tests/qemu-iotests/232
>   create mode 100644 tests/qemu-iotests/232.out

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


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 02/11] block: Add auto-read-only option
  2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 02/11] block: Add auto-read-only option Kevin Wolf
@ 2018-10-31 17:14   ` Kevin Wolf
  0 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-31 17:14 UTC (permalink / raw)
  To: qemu-block; +Cc: mreitz, eblake, pkrempa, qemu-devel

Am 19.10.2018 um 18:30 hat Kevin Wolf geschrieben:
> If a management application builds the block graph node by node, the
> protocol layer doesn't inherit its read-only option from the format
> layer any more, so it must be set explicitly.
> 
> Backing files should work on read-only storage, but at the same time, a
> block job like commit should be able to reopen them read-write if they
> are on read-write storage. However, without option inheritance, reopen
> only changes the read-only option for the root node (typically the
> format layer), but not the protocol layer, so reopening fails (the
> format layer wants to get write permissions, but the protocol layer is
> still read-only).
> 
> A simple workaround for the problem in the management tool would be to
> open the protocol layer always read-write and to make only the format
> layer read-only for backing files. However, sometimes the file is
> actually stored on read-only storage and we don't know whether the image
> can be opened read-write (for example, for NBD it depends on the server
> we're trying to connect to). This adds an option that makes QEMU try to
> open the image read-write, but allows it to degrade to a read-only mode
> without returning an error.
> 
> The documentation for this option is consciously phrased in a way that
> allows QEMU to switch to a better model eventually: Instead of trying
> when the image is first opened, making the read-only flag dynamic and
> changing it automatically whenever the first BLK_PERM_WRITE user is
> attached or the last one is detached would be much more useful
> behaviour.
> 
> Unfortunately, this more useful behaviour is also a lot harder to
> implement, and libvirt needs a solution now before it can switch to
> -blockdev, so let's start with this easier approach for now.
> 
> Instead of adding a new auto-read-only option, turning the existing
> read-only into an enum (with a bool alternate for compatibility) was
> considered, but it complicated the implementation to the point that it
> didn't seem to be worth it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

I'm squashing in the following hunk to fix qemu-iotests 118 (not
resetting the flag during blockdev-change-medium could result in an
image opened read-only and then failing when attaching it to a
read-write device - this was already an error case, but with the fix it
keeps the nicer error mode that the old image stays inserted instead of
ejecting it but failing to insert the replacement):

diff --git a/blockdev.c b/blockdev.c
index c30495d035..fc8794862f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2651,7 +2651,7 @@ void qmp_blockdev_change_medium(bool has_device, const char *device,

     bdrv_flags = blk_get_open_flags_from_root_state(blk);
     bdrv_flags &= ~(BDRV_O_TEMPORARY | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING |
-        BDRV_O_PROTOCOL);
+        BDRV_O_PROTOCOL | BDRV_O_AUTO_RDONLY);

     if (!has_read_only) {
         read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option
  2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
                   ` (10 preceding siblings ...)
  2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 11/11] qemu-iotests: Test auto-read-only with -drive and -blockdev Kevin Wolf
@ 2018-10-31 17:16 ` Kevin Wolf
  11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2018-10-31 17:16 UTC (permalink / raw)
  To: qemu-block; +Cc: mreitz, eblake, pkrempa, qemu-devel

Am 19.10.2018 um 18:30 hat Kevin Wolf geschrieben:
> See patch 2 for an explanation of the motivation.
> 
> v4:
> - Split fix for missing rbd_close() into a separate patch [Eric]
> - Added qemu-iotests case
> 
> v3:
> - Clarified QAPI schema documentation that auto-read-only can only
>   degrade read-write to read-only, not the other way round [Eric]
> - Don't refuse to set copy-on-read=on and auto-read-only=on at the same
>   time; only complain when actually trying to degrade to read-only
> - Let bdrv_apply_auto_read_only() return -EACCESS on all errors
> - Fixed file-posix and gluster implementations [Eric, Niels]
> - Added a patch to make auto-read-only=on the default for human user
>   interfaces (-drive/-hda/...)
> 
> v2:
> - Turn bdrv_set_read_only() into bdrv_apply_auto_read_only()
> - Support the option in a lot more block drivers
> 
> Kevin Wolf (11):
>   block: Update flags in bdrv_set_read_only()
>   block: Add auto-read-only option
>   rbd: Close image in qemu_rbd_open() error path
>   block: Require auto-read-only for existing fallbacks
>   nbd: Support auto-read-only option
>   file-posix: Support auto-read-only option
>   curl: Support auto-read-only option
>   gluster: Support auto-read-only option
>   iscsi: Support auto-read-only option
>   block: Make auto-read-only=on default for -drive
>   qemu-iotests: Test auto-read-only with -drive and -blockdev

Applied to the block branch (with the added hunk in patch 2).

Kevin

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-10-31 17:16 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-19 16:30 [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 01/11] block: Update flags in bdrv_set_read_only() Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 02/11] block: Add auto-read-only option Kevin Wolf
2018-10-31 17:14   ` Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 03/11] rbd: Close image in qemu_rbd_open() error path Kevin Wolf
2018-10-19 18:14   ` Eric Blake
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 04/11] block: Require auto-read-only for existing fallbacks Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 05/11] nbd: Support auto-read-only option Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 06/11] file-posix: " Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 07/11] curl: " Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 08/11] gluster: " Kevin Wolf
2018-10-19 16:30 ` [Qemu-devel] [PATCH v4 09/11] iscsi: " Kevin Wolf
2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 10/11] block: Make auto-read-only=on default for -drive Kevin Wolf
2018-10-20 19:31 ` [Qemu-devel] [PATCH v4 11/11] qemu-iotests: Test auto-read-only with -drive and -blockdev Kevin Wolf
2018-10-22 12:14   ` Eric Blake
2018-10-31 17:16 ` [Qemu-devel] [PATCH v4 00/11] block: Add auto-read-only option Kevin Wolf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.