[Qemu-devel] [PATCH 0/7] Add "read-only" to the options QDict

[Qemu-devel] [PATCH 0/7] Add "read-only" to the options QDict

[Alberto Garcia]

This series adds "read-only" to the options QDict, fixing a
long-standing problem with the reopening code.

   [E] <- [D] <- [C] <- [B] <- [A]

In a normal scenario, the active layer [A] is in read-write mode and
everything else is read-only. If we reopen [D] in read-write mode and
later reopen [B], then [D] will become read-only when inheriting the
flags from its parent (see bdrv_backing_options()). With this series,
inheriting options doesn't need to override values that have been
explicitly set before.

The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the
value of the "read-only" option. Removing the BDRV_O_RDWR flag is not
straightforward in all cases and can result in code that is
significantly slower and less readable. Therefore it will be dealt
with in the future.

Regards,

Berto

Alberto Garcia (7):
  block: Remove bdrv_is_snapshot
  block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the
    flags
  block: Update bs->open_flags earlier in bdrv_open_common()
  block: Add "read-only" to the options QDict
  block: Don't queue the same BDS twice in bdrv_reopen_queue_child()
  commit: Add 'base' to the reopen queue before 'overlay_bs'
  block: rename "read-only" to BDRV_OPT_READ_ONLY

 block.c               | 86 +++++++++++++++++++++++++++++++++++++--------------
 block/commit.c        |  8 ++---
 block/vvfat.c         |  3 +-
 blockdev.c            | 28 ++++++++++-------
 include/block/block.h |  2 +-
 5 files changed, 86 insertions(+), 41 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/7] block: Remove bdrv_is_snapshot

[Alberto Garcia]

This is unnecessary and has been unused since 5433c24f0f9306c82ad9bcc.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 5 -----
 include/block/block.h | 1 -
 2 files changed, 6 deletions(-)

diff --git a/block.c b/block.c
index 66ed1c0..1c75a6f 100644
--- a/block.c
+++ b/block.c
@@ -2965,11 +2965,6 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
     return false;
 }
 
-int bdrv_is_snapshot(BlockDriverState *bs)
-{
-    return !!(bs->open_flags & BDRV_O_SNAPSHOT);
-}
-
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/include/block/block.h b/include/block/block.h
index 7edce5c..9f0399f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -420,7 +420,6 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed,
                                                   const char *backing,
                                                   char *dest, size_t sz,
                                                   Error **errp);
-int bdrv_is_snapshot(BlockDriverState *bs);
 
 int path_has_protocol(const char *path);
 int path_is_absolute(const char *path);
-- 
2.9.3

[Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Alberto Garcia]

If an image is opened with snapshot=on, its flags are modified by
bdrv_backing_options() and then bs->open_flags is updated accordingly.
This last step is unnecessary if we calculate the new flags before
setting bs->open_flags.

Soon we'll introduce the "read-only" option, and then we'll need to be
able to modify its value in the QDict when snapshot=on. This is more
cumbersome if bs->options is already set. This patch simplifies that.

The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
reason.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 1c75a6f..7cae841 100644
--- a/block.c
+++ b/block.c
@@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         goto fail;
     }
 
+    if (flags & BDRV_O_RDWR) {
+        flags |= BDRV_O_ALLOW_RDWR;
+    }
+
+    if (flags & BDRV_O_SNAPSHOT) {
+        snapshot_options = qdict_new();
+        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
+                                   flags, options);
+        bdrv_backing_options(&flags, options, flags, options);
+    }
+
     bs->open_flags = flags;
     bs->options = options;
     options = qdict_clone_shallow(options);
@@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 
     /* Open image file without format layer */
     if ((flags & BDRV_O_PROTOCOL) == 0) {
-        if (flags & BDRV_O_RDWR) {
-            flags |= BDRV_O_ALLOW_RDWR;
-        }
-        if (flags & BDRV_O_SNAPSHOT) {
-            snapshot_options = qdict_new();
-            bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
-                                       flags, options);
-            bdrv_backing_options(&flags, options, flags, options);
-        }
-
-        bs->open_flags = flags;
-
         file = bdrv_open_child(filename, options, "file", bs,
                                &child_file, true, &local_err);
         if (local_err) {
-- 
2.9.3

[Qemu-devel] [PATCH 3/7] block: Update bs->open_flags earlier in bdrv_open_common()

[Alberto Garcia]

We're only doing this immediately before opening the image, but
bs->open_flags is used earlier in the function. At the moment this is
not causing problems because none of the checked flags are modified by
update_flags_from_options(), but this will change when we introduce
the "read-only" option.

This patch calls update_flags_from_options() at the beginning of the
function, immediately after creating the QemuOpts.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 7cae841..f56d703 100644
--- a/block.c
+++ b/block.c
@@ -913,6 +913,8 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto fail_opts;
     }
 
+    update_flags_from_options(&bs->open_flags, opts);
+
     driver_name = qemu_opt_get(opts, "driver");
     drv = bdrv_find_format(driver_name);
     assert(drv != NULL);
@@ -974,9 +976,6 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
-    /* Apply cache mode options */
-    update_flags_from_options(&bs->open_flags, opts);
-
     /* Open the image, either directly or using a protocol */
     open_flags = bdrv_open_flags(bs, bs->open_flags);
     if (drv->bdrv_file_open) {
-- 
2.9.3

[Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict

[Alberto Garcia]

This adds the "read-only" option to the QDict. One important effect of
this change is that when a child inherits options from its parent, the
existing "read-only" mode can be preserved if it was explicitly set
previously.

This addresses scenarios like this:

   [E] <- [D] <- [C] <- [B] <- [A]

In this case, if we reopen [D] with read-only=off, and later reopen
[B], then [D] will not inherit read-only=on from its parent during the
bdrv_reopen_queue_child() stage.

The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the
value of the "read-only" option.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 35 ++++++++++++++++++++++++++++++++---
 block/vvfat.c         |  3 ++-
 blockdev.c            | 16 +++++++++++-----
 include/block/block.h |  1 +
 4 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index f56d703..cf9c513 100644
--- a/block.c
+++ b/block.c
@@ -707,6 +707,9 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
 
+    /* Inherit the read-only option from the parent if it's not set */
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_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
      * corresponding parent options. */
@@ -760,7 +763,8 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
 
     /* backing files always opened read-only */
-    flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ);
+    qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
+    flags &= ~BDRV_O_COPY_ON_READ;
 
     /* snapshot=on is handled on the top layer */
     flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_TEMPORARY);
@@ -807,6 +811,14 @@ static void update_flags_from_options(int *flags, QemuOpts *opts)
     if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
         *flags |= BDRV_O_NOCACHE;
     }
+
+    *flags &= ~BDRV_O_RDWR;
+
+    assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY));
+    if (!qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false)) {
+        *flags |= BDRV_O_RDWR;
+    }
+
 }
 
 static void update_options_from_flags(QDict *options, int flags)
@@ -819,6 +831,10 @@ static void update_options_from_flags(QDict *options, int flags)
         qdict_put(options, BDRV_OPT_CACHE_NO_FLUSH,
                   qbool_from_bool(flags & BDRV_O_NO_FLUSH));
     }
+    if (!qdict_haskey(options, BDRV_OPT_READ_ONLY)) {
+        qdict_put(options, BDRV_OPT_READ_ONLY,
+                  qbool_from_bool(!(flags & BDRV_O_RDWR)));
+    }
 }
 
 static void bdrv_assign_node_name(BlockDriverState *bs,
@@ -882,6 +898,11 @@ static QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Ignore flush requests",
         },
+        {
+            .name = BDRV_OPT_READ_ONLY,
+            .type = QEMU_OPT_BOOL,
+            .help = "Node is opened in read-only mode",
+        },
         { /* end of list */ }
     },
 };
@@ -1626,14 +1647,22 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         goto fail;
     }
 
-    if (flags & BDRV_O_RDWR) {
-        flags |= BDRV_O_ALLOW_RDWR;
+    /* Set the BDRV_O_RDWR and BDRV_O_ALLOW_RDWR flags.
+     * FIXME: we're parsing the QDict to avoid having to create a
+     * QemuOpts just for this, but neither option is optimal. */
+    if (g_strcmp0(qdict_get_try_str(options, BDRV_OPT_READ_ONLY), "on") &&
+        !qdict_get_try_bool(options, BDRV_OPT_READ_ONLY, false)) {
+        flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);
+    } else {
+        flags &= ~BDRV_O_RDWR;
     }
 
     if (flags & BDRV_O_SNAPSHOT) {
         snapshot_options = qdict_new();
         bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
                                    flags, options);
+        /* Let bdrv_backing_options() override "read-only" */
+        qdict_del(options, BDRV_OPT_READ_ONLY);
         bdrv_backing_options(&flags, options, flags, options);
     }
 
diff --git a/block/vvfat.c b/block/vvfat.c
index ba2620f..ded2109 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2971,7 +2971,8 @@ static BlockDriver vvfat_write_target = {
 static void vvfat_qcow_options(int *child_flags, QDict *child_options,
                                int parent_flags, QDict *parent_options)
 {
-    *child_flags = BDRV_O_RDWR | BDRV_O_NO_FLUSH;
+    qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "off");
+    *child_flags = BDRV_O_NO_FLUSH;
 }
 
 static const BdrvChildRole child_vvfat_qcow = {
diff --git a/blockdev.c b/blockdev.c
index 1399fbd..3036af4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -360,9 +360,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     const char *aio;
 
     if (bdrv_flags) {
-        if (!qemu_opt_get_bool(opts, "read-only", false)) {
-            *bdrv_flags |= BDRV_O_RDWR;
-        }
         if (qemu_opt_get_bool(opts, "copy-on-read", false)) {
             *bdrv_flags |= BDRV_O_COPY_ON_READ;
         }
@@ -471,7 +468,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     int bdrv_flags = 0;
     int on_read_error, on_write_error;
     bool account_invalid, account_failed;
-    bool writethrough;
+    bool writethrough, read_only;
     BlockBackend *blk;
     BlockDriverState *bs;
     ThrottleConfig cfg;
@@ -567,6 +564,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         bdrv_flags |= BDRV_O_SNAPSHOT;
     }
 
+    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
+
     /* init */
     if ((!file || !*file) && !qdict_size(bs_opts)) {
         BlockBackendRootState *blk_rs;
@@ -574,7 +573,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         blk = blk_new();
         blk_rs = blk_get_root_state(blk);
         blk_rs->open_flags    = bdrv_flags;
-        blk_rs->read_only     = !(bdrv_flags & BDRV_O_RDWR);
+        blk_rs->read_only     = read_only;
         blk_rs->detect_zeroes = detect_zeroes;
 
         QDECREF(bs_opts);
@@ -588,6 +587,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
          * Apply the defaults here instead. */
         qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
         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");
         assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
         if (runstate_check(RUN_STATE_INMIGRATE)) {
@@ -657,6 +658,7 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
     QemuOpts *opts;
     Error *local_error = NULL;
     BlockdevDetectZeroesOptions detect_zeroes;
+    bool read_only;
     int bdrv_flags = 0;
 
     opts = qemu_opts_create(&qemu_root_bds_opts, NULL, 1, errp);
@@ -677,11 +679,15 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
         goto fail;
     }
 
+    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
+
     /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
      * with other callers) rather than what we want as the real defaults.
      * Apply the defaults here instead. */
     qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
     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");
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         bdrv_flags |= BDRV_O_INACTIVE;
diff --git a/include/block/block.h b/include/block/block.h
index 9f0399f..57ae122 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -107,6 +107,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_WB       "cache.writeback"
 #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_SECTOR_BITS   9
-- 
2.9.3

[Qemu-devel] [PATCH 5/7] block: Don't queue the same BDS twice in bdrv_reopen_queue_child()

[Alberto Garcia]

bdrv_reopen_queue_child() assumes that a BlockDriverState is never
added twice to BlockReopenQueue.

That's however not the case: commit_start() adds 'base' (and its
children) to a new reopen queue, and then 'overlay_bs' (and its
children, which include 'base') to the same queue. The effect of this
is that the first set of options is ignored and overriden by the
second.

We fixed this by swapping the order in which both BDSs were added to
the queue in 3db2bd5508c86a1605258bc77c9672d93b5c350e. This patch
checks if a BDS is already in the reopen queue and keeps its options.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index cf9c513..57f4b2c 100644
--- a/block.c
+++ b/block.c
@@ -1874,6 +1874,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
         options = qdict_new();
     }
 
+    /* Check if this BlockDriverState is already in the queue */
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        if (bs == bs_entry->state.bs) {
+            break;
+        }
+    }
+
     /*
      * Precedence of options:
      * 1. Explicitly passed in options (highest)
@@ -1894,7 +1901,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     }
 
     /* Old explicitly set values (don't overwrite by inherited value) */
-    old_options = qdict_clone_shallow(bs->explicit_options);
+    if (bs_entry) {
+        old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
+    } else {
+        old_options = qdict_clone_shallow(bs->explicit_options);
+    }
     bdrv_join_options(bs, options, old_options);
     QDECREF(old_options);
 
@@ -1933,8 +1944,13 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
                                 child->role, options, flags);
     }
 
-    bs_entry = g_new0(BlockReopenQueueEntry, 1);
-    QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+    if (!bs_entry) {
+        bs_entry = g_new0(BlockReopenQueueEntry, 1);
+        QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+    } else {
+        QDECREF(bs_entry->state.options);
+        QDECREF(bs_entry->state.explicit_options);
+    }
 
     bs_entry->state.bs = bs;
     bs_entry->state.options = options;
-- 
2.9.3

[Qemu-devel] [PATCH 6/7] commit: Add 'base' to the reopen queue before 'overlay_bs'

[Alberto Garcia]

Now that we're checking for duplicates in the reopen queue, there's no
need to force a specific order in which the queue is constructed so we
can revert 3db2bd5508c86a1605258bc77c9672d93b5c350e.

Since both ways of constructing the queue are now valid, this patch
doesn't have any effect on the behavior of QEMU and is not strictly
necessary. However it can help us check that the fix for the reopen
queue is robust: if it stops working properly at some point, iotest
040 will break.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 553e18d..3ab5e0c 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -243,14 +243,14 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     orig_overlay_flags = bdrv_get_flags(overlay_bs);
 
     /* convert base & overlay_bs to r/w, if necessary */
-    if (!(orig_overlay_flags & BDRV_O_RDWR)) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
-                                         orig_overlay_flags | BDRV_O_RDWR);
-    }
     if (!(orig_base_flags & BDRV_O_RDWR)) {
         reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
                                          orig_base_flags | BDRV_O_RDWR);
     }
+    if (!(orig_overlay_flags & BDRV_O_RDWR)) {
+        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
+                                         orig_overlay_flags | BDRV_O_RDWR);
+    }
     if (reopen_queue) {
         bdrv_reopen_multiple(reopen_queue, &local_err);
         if (local_err != NULL) {
-- 
2.9.3

[Qemu-devel] [PATCH 7/7] block: rename "read-only" to BDRV_OPT_READ_ONLY

[Alberto Garcia]

There were a few instances left. After this patch we're using the
macro in all places.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3036af4..6db2c53 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -811,7 +811,7 @@ QemuOptsList qemu_legacy_drive_opts = {
 
         /* Options that are passed on, but have special semantics with -drive */
         {
-            .name = "read-only",
+            .name = BDRV_OPT_READ_ONLY,
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
         },{
@@ -877,7 +877,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
         { "group",          "throttling.group" },
 
-        { "readonly",       "read-only" },
+        { "readonly",       BDRV_OPT_READ_ONLY },
     };
 
     for (i = 0; i < ARRAY_SIZE(opt_renames); i++) {
@@ -949,7 +949,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     }
 
     /* copy-on-read is disabled with a warning for read-only devices */
-    read_only |= qemu_opt_get_bool(legacy_opts, "read-only", false);
+    read_only |= qemu_opt_get_bool(legacy_opts, BDRV_OPT_READ_ONLY, false);
     copy_on_read = qemu_opt_get_bool(legacy_opts, "copy-on-read", false);
 
     if (read_only && copy_on_read) {
@@ -957,7 +957,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         copy_on_read = false;
     }
 
-    qdict_put(bs_opts, "read-only",
+    qdict_put(bs_opts, BDRV_OPT_READ_ONLY,
               qstring_from_str(read_only ? "on" : "off"));
     qdict_put(bs_opts, "copy-on-read",
               qstring_from_str(copy_on_read ? "on" :"off"));
@@ -4046,7 +4046,7 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_STRING,
             .help = "write error action",
         },{
-            .name = "read-only",
+            .name = BDRV_OPT_READ_ONLY,
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
         },{
@@ -4165,7 +4165,7 @@ static QemuOptsList qemu_root_bds_opts = {
             .type = QEMU_OPT_STRING,
             .help = "host AIO implementation (threads, native)",
         },{
-            .name = "read-only",
+            .name = BDRV_OPT_READ_ONLY,
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
         },{
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> If an image is opened with snapshot=on, its flags are modified by
> bdrv_backing_options() and then bs->open_flags is updated accordingly.
> This last step is unnecessary if we calculate the new flags before
> setting bs->open_flags.
> 
> Soon we'll introduce the "read-only" option, and then we'll need to be
> able to modify its value in the QDict when snapshot=on. This is more
> cumbersome if bs->options is already set. This patch simplifies that.
> 
> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
> reason.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c75a6f..7cae841 100644
> --- a/block.c
> +++ b/block.c
> @@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>          goto fail;
>      }
>  
> +    if (flags & BDRV_O_RDWR) {
> +        flags |= BDRV_O_ALLOW_RDWR;
> +    }
> +
> +    if (flags & BDRV_O_SNAPSHOT) {
> +        snapshot_options = qdict_new();
> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> +                                   flags, options);
> +        bdrv_backing_options(&flags, options, flags, options);
> +    }
> +
>      bs->open_flags = flags;
>      bs->options = options;
>      options = qdict_clone_shallow(options);

Here I think we get different semantics now: bdrv_backing_options() only
affected the cloned QDict before, and now it affects bs->options, too.

Given that the flags are already updated and don't contain
BDRV_O_SNAPSHOT any more, I suspect that this is a silent bug fix. If
so, it might be better to split it out into a separate patch before this
one, or at the very least should be mentioned in the commit message.

If it's not a fix, however, it's probably a bug. :-)

> @@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>  
>      /* Open image file without format layer */
>      if ((flags & BDRV_O_PROTOCOL) == 0) {
> -        if (flags & BDRV_O_RDWR) {
> -            flags |= BDRV_O_ALLOW_RDWR;
> -        }
> -        if (flags & BDRV_O_SNAPSHOT) {
> -            snapshot_options = qdict_new();
> -            bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> -                                       flags, options);
> -            bdrv_backing_options(&flags, options, flags, options);
> -        }
> -
> -        bs->open_flags = flags;
> -
>          file = bdrv_open_child(filename, options, "file", bs,
>                                 &child_file, true, &local_err);
>          if (local_err) {

Kevin

Re: [Qemu-devel] [PATCH 3/7] block: Update bs->open_flags earlier in bdrv_open_common()

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> We're only doing this immediately before opening the image, but
> bs->open_flags is used earlier in the function. At the moment this is
> not causing problems because none of the checked flags are modified by
> update_flags_from_options(), but this will change when we introduce
> the "read-only" option.
> 
> This patch calls update_flags_from_options() at the beginning of the
> function, immediately after creating the QemuOpts.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

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

Re: [Qemu-devel] [PATCH 1/7] block: Remove bdrv_is_snapshot

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> This is unnecessary and has been unused since 5433c24f0f9306c82ad9bcc.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

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

Re: [Qemu-devel] [PATCH 1/7] block: Remove bdrv_is_snapshot

[Eric Blake]

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

On 09/14/2016 10:52 AM, Alberto Garcia wrote:
> This is unnecessary and has been unused since 5433c24f0f9306c82ad9bcc.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 5 -----
>  include/block/block.h | 1 -
>  2 files changed, 6 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Jeff Cody]

On Wed, Sep 14, 2016 at 06:52:15PM +0300, Alberto Garcia wrote:
> If an image is opened with snapshot=on, its flags are modified by
> bdrv_backing_options() and then bs->open_flags is updated accordingly.
> This last step is unnecessary if we calculate the new flags before
> setting bs->open_flags.
> 
> Soon we'll introduce the "read-only" option, and then we'll need to be
> able to modify its value in the QDict when snapshot=on. This is more
> cumbersome if bs->options is already set. This patch simplifies that.
> 
> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
> reason.
> 

Before, we would not set BDRV_O_ALLOW_RDWR for protocols, but this will
change that.  Is that side-affect intentional?

> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c75a6f..7cae841 100644
> --- a/block.c
> +++ b/block.c
> @@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>          goto fail;
>      }
>  
> +    if (flags & BDRV_O_RDWR) {
> +        flags |= BDRV_O_ALLOW_RDWR;
> +    }
> +
> +    if (flags & BDRV_O_SNAPSHOT) {
> +        snapshot_options = qdict_new();
> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> +                                   flags, options);
> +        bdrv_backing_options(&flags, options, flags, options);
> +    }
> +
>      bs->open_flags = flags;
>      bs->options = options;
>      options = qdict_clone_shallow(options);
> @@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>  
>      /* Open image file without format layer */
>      if ((flags & BDRV_O_PROTOCOL) == 0) {
> -        if (flags & BDRV_O_RDWR) {
> -            flags |= BDRV_O_ALLOW_RDWR;
> -        }
> -        if (flags & BDRV_O_SNAPSHOT) {
> -            snapshot_options = qdict_new();
> -            bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> -                                       flags, options);
> -            bdrv_backing_options(&flags, options, flags, options);
> -        }
> -
> -        bs->open_flags = flags;
> -
>          file = bdrv_open_child(filename, options, "file", bs,
>                                 &child_file, true, &local_err);
>          if (local_err) {

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Alberto Garcia]

On Wed 14 Sep 2016 08:54:19 PM CEST, Jeff Cody wrote:
>> If an image is opened with snapshot=on, its flags are modified by
>> bdrv_backing_options() and then bs->open_flags is updated accordingly.
>> This last step is unnecessary if we calculate the new flags before
>> setting bs->open_flags.
>> 
>> Soon we'll introduce the "read-only" option, and then we'll need to be
>> able to modify its value in the QDict when snapshot=on. This is more
>> cumbersome if bs->options is already set. This patch simplifies that.
>> 
>> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
>> reason.
>
> Before, we would not set BDRV_O_ALLOW_RDWR for protocols, but this
> will change that.  Is that side-affect intentional?

BDRV_O_ALLOW_RDWR is set in the root BDS, and then all children inherit
that flag (both bdrv_inherited_options() and bdrv_backing_options() copy
it), so I don't think the ((flags & BDRV_O_PROTOCOL) == 0) check makes
any difference in practice.

Berto

Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> This adds the "read-only" option to the QDict. One important effect of
> this change is that when a child inherits options from its parent, the
> existing "read-only" mode can be preserved if it was explicitly set
> previously.
> 
> This addresses scenarios like this:
> 
>    [E] <- [D] <- [C] <- [B] <- [A]
> 
> In this case, if we reopen [D] with read-only=off, and later reopen
> [B], then [D] will not inherit read-only=on from its parent during the
> bdrv_reopen_queue_child() stage.
> 
> The BDRV_O_RDWR flag is not removed yet, but its keep in sync with the
> value of the "read-only" option.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 35 ++++++++++++++++++++++++++++++++---
>  block/vvfat.c         |  3 ++-
>  blockdev.c            | 16 +++++++++++-----
>  include/block/block.h |  1 +
>  4 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index f56d703..cf9c513 100644
> --- a/block.c
> +++ b/block.c
> @@ -707,6 +707,9 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
>  
> +    /* Inherit the read-only option from the parent if it's not set */
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_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
>       * corresponding parent options. */

We need another qdict_copy_default() in bdrv_temp_snapshot_options(), I
think, so that flags and options stay consistent there.

Currently it seems to be working because we still have the flag and the
flag is what is actually used in bdrv_open_common(). This means that we
parse the "read-only" option into the QemuOpts there, but we never read
it from there. Shouldn't we use the option rather than the flags?

> @@ -677,11 +679,15 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
>          goto fail;
>      }
>  
> +    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
> +
>      /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
>       * with other callers) rather than what we want as the real defaults.
>       * Apply the defaults here instead. */
>      qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
>      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");

Why do you parse read_only into the QemuOpts just to add it right back
to bs_opts? Wouldn't it be easier to remove it from qemu_root_bds_opts
and do a simple set_default_str("on") here? (Which is how the cache
options work.)

Kevin

Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Alberto Garcia]

On Wed 14 Sep 2016 06:40:29 PM CEST, Kevin Wolf wrote:
>> +    if (flags & BDRV_O_RDWR) {
>> +        flags |= BDRV_O_ALLOW_RDWR;
>> +    }
>> +
>> +    if (flags & BDRV_O_SNAPSHOT) {
>> +        snapshot_options = qdict_new();
>> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
>> +                                   flags, options);
>> +        bdrv_backing_options(&flags, options, flags, options);
>> +    }
>> +
>>      bs->open_flags = flags;
>>      bs->options = options;
>>      options = qdict_clone_shallow(options);
>
> Here I think we get different semantics now: bdrv_backing_options()
> only affected the cloned QDict before, and now it affects bs->options,
> too.

The thing is that bdrv_backing_options() doesn't change the options in
general, and in the case where it does (BDRV_OPT_READ_ONLY after the
next patch) I think it makes sense that the options are changed.
"snapshot=on" is a bit of a special case after all.

Berto

Re: [Qemu-devel] [PATCH 4/7] block: Add "read-only" to the options QDict

[Alberto Garcia]

On Thu 15 Sep 2016 12:51:27 PM CEST, Kevin Wolf wrote:
>> @@ -707,6 +707,9 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
>>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
>>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
>>  
>> +    /* Inherit the read-only option from the parent if it's not set */
>> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_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
>>       * corresponding parent options. */
>
> We need another qdict_copy_default() in bdrv_temp_snapshot_options(),
> I think, so that flags and options stay consistent there.

You're right, I assumed that with read-only=on,snapshot=on the temporary
file would still be r/w.

I'll fix it.

>> +    read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
>> +
>>      /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
>>       * with other callers) rather than what we want as the real defaults.
>>       * Apply the defaults here instead. */
>>      qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
>>      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");
>
> Why do you parse read_only into the QemuOpts just to add it right back
> to bs_opts? Wouldn't it be easier to remove it from qemu_root_bds_opts
> and do a simple set_default_str("on") here? (Which is how the cache
> options work.)

Yeah, I was mirroring blockdev_init() (there it's easier to keep it in
qemu_common_drive_opts), but in this case there's no need to do it.

I'll fix it too.

Thanks!

Berto

Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Kevin Wolf]

Am 15.09.2016 um 13:24 hat Alberto Garcia geschrieben:
> On Wed 14 Sep 2016 06:40:29 PM CEST, Kevin Wolf wrote:
> >> +    if (flags & BDRV_O_RDWR) {
> >> +        flags |= BDRV_O_ALLOW_RDWR;
> >> +    }
> >> +
> >> +    if (flags & BDRV_O_SNAPSHOT) {
> >> +        snapshot_options = qdict_new();
> >> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> >> +                                   flags, options);
> >> +        bdrv_backing_options(&flags, options, flags, options);
> >> +    }
> >> +
> >>      bs->open_flags = flags;
> >>      bs->options = options;
> >>      options = qdict_clone_shallow(options);
> >
> > Here I think we get different semantics now: bdrv_backing_options()
> > only affected the cloned QDict before, and now it affects bs->options,
> > too.
> 
> The thing is that bdrv_backing_options() doesn't change the options in
> general, and in the case where it does (BDRV_OPT_READ_ONLY after the
> next patch) I think it makes sense that the options are changed.
> "snapshot=on" is a bit of a special case after all.

Fair enough, it is correct (as I suspected), but it's not a bug fix
because so far the options weren't changed, so there's no observable
difference.

I think mentioning this in the commit message wouldn't hurt, though.

Kevin

Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Alberto Garcia]

On Thu 15 Sep 2016 02:19:36 PM CEST, Kevin Wolf wrote:

>> >> +    if (flags & BDRV_O_RDWR) {
>> >> +        flags |= BDRV_O_ALLOW_RDWR;
>> >> +    }
>> >> +
>> >> +    if (flags & BDRV_O_SNAPSHOT) {
>> >> +        snapshot_options = qdict_new();
>> >> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
>> >> +                                   flags, options);
>> >> +        bdrv_backing_options(&flags, options, flags, options);
>> >> +    }
>> >> +
>> >>      bs->open_flags = flags;
>> >>      bs->options = options;
>> >>      options = qdict_clone_shallow(options);
>> >
>> > Here I think we get different semantics now: bdrv_backing_options()
>> > only affected the cloned QDict before, and now it affects bs->options,
>> > too.
>> 
>> The thing is that bdrv_backing_options() doesn't change the options in
>> general, and in the case where it does (BDRV_OPT_READ_ONLY after the
>> next patch) I think it makes sense that the options are changed.
>> "snapshot=on" is a bit of a special case after all.
>
> Fair enough, it is correct (as I suspected), but it's not a bug fix
> because so far the options weren't changed, so there's no observable
> difference.

The idea of this change is to remove the ' bs->open_flags = flags ' line
in the original code, as explained in the commit message.

That's one thing. The other (and most important one) is to prepare for
the introduction of BDRV_OPT_READ_ONLY.

Without this patch we'd need to need to update not just bs->open_flags
but bs->options as well, and it would be something like this:

   bs->open_flags = flags;
   bs->options = options;

      /* ... */

   if ((flags & BDRV_O_PROTOCOL) == 0) {

      /* ... */

      if (flags & BDRV_O_SNAPSHOT) {
          snapshot_options = qdict_new();
          bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
                                     flags, options);
+         qdict_del(bs->options, BDRV_OPT_READ_ONLY);
+         qdict_del(options, BDRV_OPT_READ_ONLY);
          bdrv_backing_options(&flags, options, flags, options);
      }

      bs->open_flags = flags;
+     qdict_copy_default(bs->options, options, BDRV_OPT_READ_ONLY);

      /* ... */
    }

Moving this chunk of code saves us from having to update bs->open_flags
and bs->options and makes things more readable.

Berto

Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Kevin Wolf]

Am 15.09.2016 um 14:31 hat Alberto Garcia geschrieben:
> On Thu 15 Sep 2016 02:19:36 PM CEST, Kevin Wolf wrote:
> 
> >> >> +    if (flags & BDRV_O_RDWR) {
> >> >> +        flags |= BDRV_O_ALLOW_RDWR;
> >> >> +    }
> >> >> +
> >> >> +    if (flags & BDRV_O_SNAPSHOT) {
> >> >> +        snapshot_options = qdict_new();
> >> >> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> >> >> +                                   flags, options);
> >> >> +        bdrv_backing_options(&flags, options, flags, options);
> >> >> +    }
> >> >> +
> >> >>      bs->open_flags = flags;
> >> >>      bs->options = options;
> >> >>      options = qdict_clone_shallow(options);
> >> >
> >> > Here I think we get different semantics now: bdrv_backing_options()
> >> > only affected the cloned QDict before, and now it affects bs->options,
> >> > too.
> >> 
> >> The thing is that bdrv_backing_options() doesn't change the options in
> >> general, and in the case where it does (BDRV_OPT_READ_ONLY after the
> >> next patch) I think it makes sense that the options are changed.
> >> "snapshot=on" is a bit of a special case after all.
> >
> > Fair enough, it is correct (as I suspected), but it's not a bug fix
> > because so far the options weren't changed, so there's no observable
> > difference.
> 
> The idea of this change is to remove the ' bs->open_flags = flags ' line
> in the original code, as explained in the commit message.
> 
> That's one thing. The other (and most important one) is to prepare for
> the introduction of BDRV_OPT_READ_ONLY.
> 
> Without this patch we'd need to need to update not just bs->open_flags
> but bs->options as well, and it would be something like this:
> 
>    bs->open_flags = flags;
>    bs->options = options;
> 
>       /* ... */
> 
>    if ((flags & BDRV_O_PROTOCOL) == 0) {
> 
>       /* ... */
> 
>       if (flags & BDRV_O_SNAPSHOT) {
>           snapshot_options = qdict_new();
>           bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
>                                      flags, options);
> +         qdict_del(bs->options, BDRV_OPT_READ_ONLY);
> +         qdict_del(options, BDRV_OPT_READ_ONLY);
>           bdrv_backing_options(&flags, options, flags, options);
>       }
> 
>       bs->open_flags = flags;
> +     qdict_copy_default(bs->options, options, BDRV_OPT_READ_ONLY);
> 
>       /* ... */
>     }
> 
> Moving this chunk of code saves us from having to update bs->open_flags
> and bs->options and makes things more readable.

I fully understand understand what the patch is for, and I don't want
you to change any code. I was asking whether there was a hidden bug fix
in here (which should have been moved into a separate patch then), but
you explained to me why there isn't, so we're good. The only thing I'm
looking for now is putting this explanation into the commit message.

So just include in the commit message that moving across the clone of
bs->options doesn't make an observable difference (at the point of this
patch) because the bdrv_backing_options() call doesn't actually change
options yet, and that applying any changes made by it in the future
to bs->options makes more sense anyway.

Kevin

Re: [Qemu-devel] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Alberto Garcia]

On Thu 15 Sep 2016 02:50:53 PM CEST, Kevin Wolf wrote:

>> Moving this chunk of code saves us from having to update
>> bs->open_flags and bs->options and makes things more readable.
>
> I fully understand understand what the patch is for

I see, I thought it wasn't clear, I'll update the message then :)

Berto

Re: [Qemu-devel] [PATCH 5/7] block: Don't queue the same BDS twice in bdrv_reopen_queue_child()

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> bdrv_reopen_queue_child() assumes that a BlockDriverState is never
> added twice to BlockReopenQueue.
> 
> That's however not the case: commit_start() adds 'base' (and its
> children) to a new reopen queue, and then 'overlay_bs' (and its
> children, which include 'base') to the same queue. The effect of this
> is that the first set of options is ignored and overriden by the
> second.
> 
> We fixed this by swapping the order in which both BDSs were added to
> the queue in 3db2bd5508c86a1605258bc77c9672d93b5c350e. This patch
> checks if a BDS is already in the reopen queue and keeps its options.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

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

Re: [Qemu-devel] [PATCH 6/7] commit: Add 'base' to the reopen queue before 'overlay_bs'

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> Now that we're checking for duplicates in the reopen queue, there's no
> need to force a specific order in which the queue is constructed so we
> can revert 3db2bd5508c86a1605258bc77c9672d93b5c350e.
> 
> Since both ways of constructing the queue are now valid, this patch
> doesn't have any effect on the behavior of QEMU and is not strictly
> necessary. However it can help us check that the fix for the reopen
> queue is robust: if it stops working properly at some point, iotest
> 040 will break.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

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

Re: [Qemu-devel] [PATCH 7/7] block: rename "read-only" to BDRV_OPT_READ_ONLY

[Kevin Wolf]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> There were a few instances left. After this patch we're using the
> macro in all places.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

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

> @@ -4165,7 +4165,7 @@ static QemuOptsList qemu_root_bds_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "host AIO implementation (threads, native)",
>          },{
> -            .name = "read-only",
> +            .name = BDRV_OPT_READ_ONLY,
>              .type = QEMU_OPT_BOOL,
>              .help = "open drive file as read-only",
>          },{

This hunk will go away with the change you're going to make to the
earlier patch, but you can keep my R-b.

Kevin