[Qemu-devel] [PATCH v2 0/8] block: Add auto-read-only option

[Qemu-devel] [PATCH v2 0/8] block: Add auto-read-only option

[Kevin Wolf]

See patch 2 for an explanation of the motivation.

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

Kevin Wolf (8):
  block: Update flags in bdrv_set_read_only()
  block: Add auto-read-only option
  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

 qapi/block-core.json  |  6 +++++
 include/block/block.h |  5 +++-
 block.c               | 53 +++++++++++++++++++++++++++++++++++--------
 block/bochs.c         | 17 +++++---------
 block/cloop.c         | 16 ++++---------
 block/curl.c          |  8 +++----
 block/dmg.c           | 16 ++++---------
 block/file-posix.c    | 13 +++++++++++
 block/gluster.c       |  9 ++++++++
 block/iscsi.c         |  8 ++++---
 block/nbd-client.c    | 10 ++++----
 block/rbd.c           | 14 ++++--------
 block/vvfat.c         | 13 ++++-------
 13 files changed, 114 insertions(+), 74 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH v2 1/8] block: Update flags in bdrv_set_read_only()

[Kevin Wolf]

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

[Qemu-devel] [PATCH v2 2/8] block: Add auto-read-only option

[Kevin Wolf]

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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json  |  6 ++++++
 include/block/block.h |  2 ++
 block.c               | 21 ++++++++++++++++++++-
 block/vvfat.c         |  1 +
 4 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index cfb37f8c1d..3a899298de 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3651,6 +3651,11 @@
 #                 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, QEMU may ignore the @read-only option and
+#                  automatically decide whether to open the image read-only or
+#                  read-write (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).
 # @detect-zeroes: detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 # @force-share:   force share all permission on added nodes.
@@ -3666,6 +3671,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..f999393e28 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,
@@ -1430,7 +1445,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     assert(atomic_read(&bs->copy_on_read) == 0);
 
     if (bs->open_flags & BDRV_O_COPY_ON_READ) {
-        if (!bs->read_only) {
+        if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_AUTO_RDONLY))
+            == BDRV_O_RDWR)
+        {
             bdrv_enable_copy_on_read(bs);
         } else {
             error_setg(errp, "Can't use copy-on-read on read-only device");
@@ -2486,6 +2503,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

[Qemu-devel] [PATCH v2 3/8] block: Require auto-read-only for existing fallbacks

[Kevin Wolf]

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>
---
 include/block/block.h |  3 ++-
 block.c               | 37 +++++++++++++++++++++++--------------
 block/bochs.c         | 17 ++++++-----------
 block/cloop.c         | 16 +++++-----------
 block/dmg.c           | 16 +++++-----------
 block/rbd.c           | 14 ++++----------
 block/vvfat.c         | 12 +++---------
 7 files changed, 48 insertions(+), 67 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 f999393e28..631501bcae 100644
--- a/block.c
+++ b/block.c
@@ -266,27 +266,36 @@ 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.
+ * 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 (!(bs->open_flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
+        error_setg(errp, "%s", errmsg ?: "Image is read-only");
+        return -EACCES;
+    }
+
+    ret = bdrv_can_set_read_only(bs, true, false, errp);
     if (ret < 0) {
         return ret;
     }
 
-    bs->read_only = read_only;
-
-    if (read_only) {
-        bs->open_flags &= ~BDRV_O_RDWR;
-    } else {
-        bs->open_flags |= BDRV_O_RDWR;
-    }
+    bs->read_only = true;
+    bs->open_flags &= ~BDRV_O_RDWR;
 
     return 0;
 }
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 014c68d629..ee0b4a6941 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -780,16 +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) {
-                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..fd814c39c9 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1262,16 +1262,10 @@ 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;
+            return ret;
         }
     }
 
-- 
2.19.1

[Qemu-devel] [PATCH v2 4/8] nbd: Support auto-read-only option

[Kevin Wolf]

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

[Qemu-devel] [PATCH v2 5/8] file-posix: Support auto-read-only option

[Kevin Wolf]

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>
---
 block/file-posix.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

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

[Qemu-devel] [PATCH v2 6/8] curl: Support auto-read-only option

[Kevin Wolf]

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

Signed-off-by: Kevin Wolf <kwolf@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

[Qemu-devel] [PATCH v2 7/8] gluster: Support auto-read-only option

[Kevin Wolf]

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>
---
 block/gluster.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..68d20c8830 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -849,6 +849,15 @@ 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 && errno == EACCES) {
+        /* Try to degrade to read-only, but if it doesn't work, still use the
+         * normal error message. */
+        ret = bdrv_apply_auto_read_only(bs, NULL, NULL);
+        if (ret == 0) {
+            open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
+            s->fd = glfs_open(s->glfs, gconf->path, open_flags);
+        }
+    }
     if (!s->fd) {
         ret = -errno;
     }
-- 
2.19.1

[Qemu-devel] [PATCH v2 8/8] iscsi: Support auto-read-only option

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v2 4/8] nbd: Support auto-read-only option

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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>
> ---
>   block/nbd-client.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 

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

> 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;
> 

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

Re: [Qemu-devel] [PATCH v2 1/8] block: Update flags in bdrv_set_read_only()

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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>
> ---
>   block.c | 7 +++++++
>   1 file changed, 7 insertions(+)

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

> 
> 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;
>   }
>   
> 

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

Re: [Qemu-devel] [PATCH v2 2/8] block: Add auto-read-only option

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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.
> 

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

I agree both with the approach of getting the simpler implementation in 
now (always writable, even when we don't need to write) as well as 
wording the documentation to permit a future stricter approach (only 
writable at the points where we need to write).

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qapi/block-core.json  |  6 ++++++
>   include/block/block.h |  2 ++
>   block.c               | 21 ++++++++++++++++++++-
>   block/vvfat.c         |  1 +
>   4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index cfb37f8c1d..3a899298de 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3651,6 +3651,11 @@
>   #                 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, QEMU may ignore the @read-only option and
> +#                  automatically decide whether to open the image read-only or
> +#                  read-write (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).

Bike-shedding: Do we really want to ignore @read-only? Here's the table 
of 9 combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows 
that must be preserved for back-compat:

RO   Auto   effect
o    o      *open for write, fail if not possible
f    o      *open for write, fail if not possible
t    o      *open for read, no conversion to write
o    f      open for write, fail if not possible
f    f      open for write, fail if not possible
t    f      open for read, no conversion to write
o    t      attempt write but graceful fall back to read
f    t      attempt write but graceful fall back to read
t    t      ignore RO flag, attempt write anyway

That last row is weird, why not make it an explicit error instead of 
ignoring the implied difference in semantics between the two?

Or, another idea: is it worth trying to support a single tri-state 
member (via an alternative between bool and enum, since the existing 
code uses a JSON bool):

"read-only": false (open for write, fail if not possible)
"read-only": true (open read-only, no later switching)
"read-only": "auto" (switch as needed; or for initial implementation 
attempt for write with graceful fallback to read)
omitting read-only: same as "read-only":false for back-compat


> @@ -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",
> +        },

If we keep your current approach, is it worth mentioning that 
auto-read-only true overrides read-only true?

The code looks okay, but I'd like discussion on the bikeshed points 
before giving R-b.

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

Re: [Qemu-devel] [PATCH v2 3/8] block: Require auto-read-only for existing fallbacks

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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>
> ---

> +++ b/block.c
> @@ -266,27 +266,36 @@ 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.
> + * If @errmsg is not NULL, it is used as the error message for the Error
> + * object.

I like it.

Worth documenting the -EINVAL (copy-on-read prevents setting read-only) 
failure as well?  (The -EPERM failure of bdrv_can_set_read_only() is not 
reachable, since this new function never clears readonly).

> + */
> +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 (!(bs->open_flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
> +        error_setg(errp, "%s", errmsg ?: "Image is read-only");
> +        return -EACCES;
> +    }
> +
> +    ret = bdrv_can_set_read_only(bs, true, false, errp);
>       if (ret < 0) {
>           return ret;
>       }

Makes sense.

> +++ b/block/vvfat.c
> @@ -1262,16 +1262,10 @@ 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;
> +            return ret;

Don't you still need the goto fail, to avoid leaking opts?

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

Re: [Qemu-devel] [PATCH v2 5/8] file-posix: Support auto-read-only option

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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>
> ---
>   block/file-posix.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 2da3a76355..eead3f2df3 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -527,6 +527,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>   
>       s->fd = -1;
>       fd = qemu_open(filename, s->open_flags, 0644);
> +
> +    if (fd < 0 && (errno == EACCES || errno == EROFS)) {
> +        /* Try to degrade to read-only, but if it doesn't work, still use the
> +         * normal error message. */
> +        ret = bdrv_apply_auto_read_only(bs, NULL, NULL);

No guarantees what errno is after this call...

> +        if (ret == 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);
> +        }
> +    }
> +
>       if (fd < 0) {
>           ret = -errno;
>           error_setg_errno(errp, errno, "Could not open '%s'", filename);

...even though you still rely on it here.

Possible solution:

fd = qemu_open();
if (fd < 0) {
     ret = -errno;
     if ((errno == EACCES || errno == EROFS) &&
         !bdrv_apply_auto_read_only(bs, NULL, NULL)) {
         bdrv_flags ...
         fd = qemu_open();
         ret = fd < 0 ? -errno : 0;
     }
}
if (fd < 0) {
     error_setg_errno(errp, -ret, "Could not open '%s'

Another possible solution: change the contract of 
bdrv_apply_auto_read_only() to leave errno unchanged (a bit odd, if the 
return value is different than the incoming errno value).

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

Re: [Qemu-devel] [PATCH v2 6/8] curl: Support auto-read-only option

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> If read-only=off, but auto-read-only=on is given, just degrade to
> read-only.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/curl.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v2 7/8] gluster: Support auto-read-only option

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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>
> ---
>   block/gluster.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 4fd55a9cc5..68d20c8830 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -849,6 +849,15 @@ 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 && errno == EACCES) {

EROFS is not possible as it was for posix file?

> +        /* Try to degrade to read-only, but if it doesn't work, still use the
> +         * normal error message. */
> +        ret = bdrv_apply_auto_read_only(bs, NULL, NULL);

No guarantees on what errno is on failure...

> +        if (ret == 0) {
> +            open_flags = (open_flags & ~O_RDWR) | O_RDONLY;
> +            s->fd = glfs_open(s->glfs, gconf->path, open_flags);
> +        }
> +    }
>       if (!s->fd) {
>           ret = -errno;

...but you are relying on it here.  (Same story as in the posix driver)

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

Re: [Qemu-devel] [PATCH v2 8/8] iscsi: Support auto-read-only option

[Eric Blake]

On 10/12/18 6:55 AM, Kevin Wolf wrote:
> 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>
> ---
>   block/iscsi.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 

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

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] gluster: Support auto-read-only option

[Niels de Vos]

On Fri, Oct 12, 2018 at 12:31:21PM -0500, Eric Blake wrote:
> On 10/12/18 6:55 AM, Kevin Wolf wrote:
> > 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>
> > ---
> >   block/gluster.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/block/gluster.c b/block/gluster.c
> > index 4fd55a9cc5..68d20c8830 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -849,6 +849,15 @@ 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 && errno == EACCES) {
> 
> EROFS is not possible as it was for posix file?

EROFS can happen, depending on the configuration of the Gluster volume.
In that case, opening read-only should work fine.

Niels

Re: [Qemu-devel] [PATCH v2 2/8] block: Add auto-read-only option

[Kevin Wolf]

Am 12.10.2018 um 18:47 hat Eric Blake geschrieben:
> On 10/12/18 6:55 AM, Kevin Wolf wrote:
> > 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.
> > 
> 
> > 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.
> 
> I agree both with the approach of getting the simpler implementation in now
> (always writable, even when we don't need to write) as well as wording the
> documentation to permit a future stricter approach (only writable at the
> points where we need to write).
> 
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   qapi/block-core.json  |  6 ++++++
> >   include/block/block.h |  2 ++
> >   block.c               | 21 ++++++++++++++++++++-
> >   block/vvfat.c         |  1 +
> >   4 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index cfb37f8c1d..3a899298de 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3651,6 +3651,11 @@
> >   #                 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, QEMU may ignore the @read-only option and
> > +#                  automatically decide whether to open the image read-only or
> > +#                  read-write (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).
> 
> Bike-shedding: Do we really want to ignore @read-only? Here's the table of 9
> combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows that must be
> preserved for back-compat:
> 
> RO   Auto   effect
> o    o      *open for write, fail if not possible
> f    o      *open for write, fail if not possible
> t    o      *open for read, no conversion to write
> o    f      open for write, fail if not possible
> f    f      open for write, fail if not possible
> t    f      open for read, no conversion to write
> o    t      attempt write but graceful fall back to read
> f    t      attempt write but graceful fall back to read
> t    t      ignore RO flag, attempt write anyway
> 
> That last row is weird, why not make it an explicit error instead of
> ignoring the implied difference in semantics between the two?

You're right that the description allows this. In practice,
auto-read-only can only make a node go from rw to ro, not the other way
round.

So our options are to document the current behaviour (auto-read-only has
no effect when the image is already read-only) or to make it an error.

One thought I had is that for convenience options like -hda (or in fact
-drive), auto-read-only=on could be the default, and only -blockdev and
blockdev-add would disable it by default. That would suggest that we
don't want to make it an error.

> Or, another idea: is it worth trying to support a single tri-state member
> (via an alternative between bool and enum, since the existing code uses a
> JSON bool):
> 
> "read-only": false (open for write, fail if not possible)
> "read-only": true (open read-only, no later switching)
> "read-only": "auto" (switch as needed; or for initial implementation attempt
> for write with graceful fallback to read)
> omitting read-only: same as "read-only":false for back-compat

If read-only were new, I would probably make it an enum, but adding it
now isn't very practical. I did actually start with an alternate and it
just wasn't very nice. One thing I remember is places that directly
accessed the options QDict, for which you could now have either a bool, a
string, an int or not present. It becomes a bit too much.

As read-only is optional, we could make it true/false/absent without
introducing an alternate and the additional int/string options, but I
don't like that very much either.


While we're talking about the schema, another thing I considered was
making auto-read-only an option only for the specific drivers that
support it so introspection could tell the management tool whether the
functionality is available. However, if we do this, we can't parse it in
block.c code and use a flag any more, but need to parse it in each
driver individually. Maybe it would be a better design anyway?

> > @@ -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",
> > +        },
> 
> If we keep your current approach, is it worth mentioning that
> auto-read-only true overrides read-only true?

This help text is never printed anywhere anyway... Maybe we should just
delete it. What we refer to is the QAPI documentation anyway.

Kevin

Re: [Qemu-devel] [PATCH v2 3/8] block: Require auto-read-only for existing fallbacks

[Kevin Wolf]

Am 12.10.2018 um 19:02 hat Eric Blake geschrieben:
> On 10/12/18 6:55 AM, Kevin Wolf wrote:
> > 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>
> > ---
> 
> > +++ b/block.c
> > @@ -266,27 +266,36 @@ 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.
> > + * If @errmsg is not NULL, it is used as the error message for the Error
> > + * object.
> 
> I like it.
> 
> Worth documenting the -EINVAL (copy-on-read prevents setting read-only)
> failure as well?  (The -EPERM failure of bdrv_can_set_read_only() is not
> reachable, since this new function never clears readonly).

In fact, -EINVAL and the error string from bdrv_can_set_read_only() may
be confusing because the user didn't explicitly request a read-only
image. Maybe it would be better to just turn this case into -EACCES with
the same error message.

What do you think?

> > + */
> > +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 (!(bs->open_flags & BDRV_O_RDWR)) {
> > +        return 0;
> > +    }
> > +    if (!(bs->open_flags & BDRV_O_AUTO_RDONLY)) {
> > +        error_setg(errp, "%s", errmsg ?: "Image is read-only");
> > +        return -EACCES;
> > +    }
> > +
> > +    ret = bdrv_can_set_read_only(bs, true, false, errp);
> >       if (ret < 0) {
> >           return ret;
> >       }
> 
> Makes sense.
> 
> > +++ b/block/vvfat.c
> > @@ -1262,16 +1262,10 @@ 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;
> > +            return ret;
> 
> Don't you still need the goto fail, to avoid leaking opts?

Yes, I do. Thanks.

Kevin

Re: [Qemu-devel] [PATCH v2 2/8] block: Add auto-read-only option

[Eric Blake]

On 10/15/18 4:37 AM, Kevin Wolf wrote:
> Am 12.10.2018 um 18:47 hat Eric Blake geschrieben:
>> On 10/12/18 6:55 AM, Kevin Wolf wrote:
>>> 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.
>>>
>>

>>
>> Bike-shedding: Do we really want to ignore @read-only? Here's the table of 9
>> combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows that must be
>> preserved for back-compat:
>>
>> RO   Auto   effect
>> o    o      *open for write, fail if not possible
>> f    o      *open for write, fail if not possible
>> t    o      *open for read, no conversion to write
>> o    f      open for write, fail if not possible
>> f    f      open for write, fail if not possible
>> t    f      open for read, no conversion to write
>> o    t      attempt write but graceful fall back to read
>> f    t      attempt write but graceful fall back to read
>> t    t      ignore RO flag, attempt write anyway
>>
>> That last row is weird, why not make it an explicit error instead of
>> ignoring the implied difference in semantics between the two?
> 
> You're right that the description allows this. In practice,
> auto-read-only can only make a node go from rw to ro, not the other way
> round.
> 
> So our options are to document the current behaviour (auto-read-only has
> no effect when the image is already read-only) or to make it an error.

Ah, that's different. I was reading it as "auto-read-only true lets you 
write if possible, overriding an explicit readonly request", while you 
are reading it as "auto-read-only true allows graceful fallback to 
read-only, and is thus a no-op if you already requested readonly"

I like yours better, so it's just a matter of coming up with the correct 
documentation wording.

> 
> One thought I had is that for convenience options like -hda (or in fact
> -drive), auto-read-only=on could be the default, and only -blockdev and
> blockdev-add would disable it by default. That would suggest that we
> don't want to make it an error.

Yes, having convenience options set auto-read-only would not be too 
terrible (since those are already magic and designed for short-hand 
human use), as long as the low-level QMP commands don't add the magic 
(explicit control is better at the low levels).

> 
>> Or, another idea: is it worth trying to support a single tri-state member
>> (via an alternative between bool and enum, since the existing code uses a
>> JSON bool):
>>
>> "read-only": false (open for write, fail if not possible)
>> "read-only": true (open read-only, no later switching)
>> "read-only": "auto" (switch as needed; or for initial implementation attempt
>> for write with graceful fallback to read)
>> omitting read-only: same as "read-only":false for back-compat
> 
> If read-only were new, I would probably make it an enum, but adding it
> now isn't very practical. I did actually start with an alternate and it
> just wasn't very nice. One thing I remember is places that directly
> accessed the options QDict, for which you could now have either a bool, a
> string, an int or not present. It becomes a bit too much.

Fair enough. Maybe it's worth a commit message note that we at least 
considered and rejected alternate implementations.

> 
> As read-only is optional, we could make it true/false/absent without
> introducing an alternate and the additional int/string options, but I
> don't like that very much either.

No, that way is not introspectible.  Adding auto-read-only is much 
friendlier.

> 
> 
> While we're talking about the schema, another thing I considered was
> making auto-read-only an option only for the specific drivers that
> support it so introspection could tell the management tool whether the
> functionality is available. However, if we do this, we can't parse it in
> block.c code and use a flag any more, but need to parse it in each
> driver individually. Maybe it would be a better design anyway?

Which drivers do you have in mind? Ones like file-posix, gluster, and 
NBD that actually have a notion of opening either read-write or 
read-only, or others that are read-only no matter what?

I'm still not convinced that a per-driver option is smart, and am 
reasonably happy with you adding it globally.

> 
>>> @@ -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",
>>> +        },
>>
>> If we keep your current approach, is it worth mentioning that
>> auto-read-only true overrides read-only true?
> 
> This help text is never printed anywhere anyway... Maybe we should just
> delete it. What we refer to is the QAPI documentation anyway.

Are you sure it never gets printed, with some of the recent patches 
around trying to improve help output?

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

Re: [Qemu-devel] [PATCH v2 3/8] block: Require auto-read-only for existing fallbacks

[Eric Blake]

On 10/16/18 9:12 AM, Kevin Wolf wrote:
> Am 12.10.2018 um 19:02 hat Eric Blake geschrieben:
>> On 10/12/18 6:55 AM, Kevin Wolf wrote:
>>> 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>
>>> ---
>>

>> Worth documenting the -EINVAL (copy-on-read prevents setting read-only)
>> failure as well?  (The -EPERM failure of bdrv_can_set_read_only() is not
>> reachable, since this new function never clears readonly).
> 
> In fact, -EINVAL and the error string from bdrv_can_set_read_only() may
> be confusing because the user didn't explicitly request a read-only
> image. Maybe it would be better to just turn this case into -EACCES with
> the same error message.
> 
> What do you think?

So, how would it trigger in practice? The user requests a copy-on-read 
action with the BDS as destination (thus the BDS must be writable, and 
can't be set to readonly); they omitted read-only (because they know 
they want copy-on-read); they supplied auto-read-only=true (because they 
are lazy and want to always use that flag if it is available); but the 
particular BDS they selected is not writable (whether read-only file 
system, read-only NBD server, etc).  In short, we can't grant them 
read-write to begin with, and can't gracefully fall back to read-only 
because it would violate their request for copy-on-read, so as long as 
we give them a sane error message about their request being impossible, 
we're good.  Yes, -EACCES sounds reasonable, if you want to code that in.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/8] block: Update flags in bdrv_set_read_only()

[Alberto Garcia]

On Fri 12 Oct 2018 01:55:25 PM CEST, Kevin Wolf wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto