[Qemu-devel] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Qemu-devel] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Alberto Garcia]

Hi all,

when reopening a BlockDriverState using bdrv_reopen() and friends the
new options can be specified either with a QDict or with flags. Both
methods overlap and that makes the semantics and the implementation
unnecessarily complicated.

This series removes the 'flags' parameter from these functions, so
from now on all option changes must be specified using a QDict. Apart
from simplifying the API, a few bugs are fixed along the way. See the
individual patches for more details.

This was tested with the current master (460f0236c12a86a38692c12d9bf).

Regards,

Berto

v5:
- Patch 14: Update commit message and add test for auto-read-only [Max]
- Patch 16: Fix update of BDRV_O_AUTO_RDONLY [Max]

v4: https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00228.html
- Patch 14: Don't assert if BDRV_OPT_AUTO_READ_ONLY is missing, e.g.
  qemu-io -c 'reopen -o auto-read-only=foo' -f raw null-co://

v3: https://lists.gnu.org/archive/html/qemu-block/2018-10/msg00648.html
- Patch 1 [from v2] has been removed and all other patch numbers are
  shifted.
- Patches 10 and 13: Fixed rebase conflicts after the patch removal.
- Patch 14: Replacement for the removed patch using a different approach.
- Patch 15: Document a non-obvious call to update_flags_from_options()

v2: https://lists.gnu.org/archive/html/qemu-block/2018-10/msg00534.html
- Patches 4 and 9: Fixed trivial rebase conflict
- Patch 11: Update messages [Max]
- Patch 12: Add comment and use bdrv_is_read_only() instead of
            using the flags directly [Max]
- Patch 14: Remove inner block and move all variable declarations to
            the beginning of the function [Max]

v1: https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00483.html
- Initial version

Output of backport-diff against v4:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/16:[----] [--] 'block: Add bdrv_reopen_set_read_only()'
002/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()'
003/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in commit_start/complete()'
004/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_commit()'
005/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in stream_start/complete()'
006/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()'
007/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()'
008/16:[----] [--] 'block: Use bdrv_reopen_set_read_only() in the mirror driver'
009/16:[----] [--] 'block: Drop bdrv_reopen()'
010/16:[----] [--] 'qemu-io: Put flag changes in the options QDict in reopen_f()'
011/16:[----] [--] 'block: Clean up reopen_backing_file() in block/replication.c'
012/16:[----] [--] 'block: Remove flags parameter from bdrv_reopen_queue()'
013/16:[----] [--] 'block: Stop passing flags to bdrv_reopen_queue_child()'
014/16:[0002] [FC] 'block: Remove assertions from update_flags_from_options()'
015/16:[----] [--] 'block: Assert that flags are up-to-date in bdrv_reopen_prepare()'
016/16:[down] 'block: Fix update of BDRV_O_AUTO_RDONLY in update_flags_from_options()'

Alberto Garcia (16):
  block: Add bdrv_reopen_set_read_only()
  block: Use bdrv_reopen_set_read_only() in
    bdrv_backing_update_filename()
  block: Use bdrv_reopen_set_read_only() in commit_start/complete()
  block: Use bdrv_reopen_set_read_only() in bdrv_commit()
  block: Use bdrv_reopen_set_read_only() in stream_start/complete()
  block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()
  block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
  block: Use bdrv_reopen_set_read_only() in the mirror driver
  block: Drop bdrv_reopen()
  qemu-io: Put flag changes in the options QDict in reopen_f()
  block: Clean up reopen_backing_file() in block/replication.c
  block: Remove flags parameter from bdrv_reopen_queue()
  block: Stop passing flags to bdrv_reopen_queue_child()
  block: Remove assertions from update_flags_from_options()
  block: Assert that flags are up-to-date in bdrv_reopen_prepare()
  block: Fix update of BDRV_O_AUTO_RDONLY in update_flags_from_options()

 block.c                    | 93 ++++++++++++++++++++--------------------------
 block/commit.c             | 23 +++++-------
 block/mirror.c             | 19 ++++++----
 block/replication.c        | 43 ++++++++++-----------
 block/stream.c             | 20 +++++-----
 blockdev.c                 | 11 ++----
 include/block/block.h      |  6 +--
 qemu-io-cmds.c             | 29 ++++++++++++++-
 tests/qemu-iotests/133     | 18 +++++++++
 tests/qemu-iotests/133.out | 15 ++++++++
 10 files changed, 156 insertions(+), 121 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v5 01/16] block: Add bdrv_reopen_set_read_only()

[Alberto Garcia]

Most callers of bdrv_reopen() only use it to switch a BlockDriverState
between read-only and read-write, so this patch adds a new function
that does just that.

We also want to get rid of the flags parameter in the bdrv_reopen()
API, so this function sets the "read-only" option and passes the
original flags (which will then be updated in bdrv_reopen_prepare()).

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 17 +++++++++++++++++
 include/block/block.h |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/block.c b/block.c
index fd67e14dfa..4e82c71ba4 100644
--- a/block.c
+++ b/block.c
@@ -3127,6 +3127,23 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
     return ret;
 }
 
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+                              Error **errp)
+{
+    int ret;
+    BlockReopenQueue *queue;
+    QDict *opts = qdict_new();
+
+    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+    bdrv_subtree_drained_begin(bs);
+    queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+    ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
+    bdrv_subtree_drained_end(bs);
+
+    return ret;
+}
+
 static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
                                                           BdrvChild *c)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 7f5453b45b..382e6643fc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -303,6 +303,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     QDict *options, int flags);
 int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
+int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
+                              Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
                         BlockReopenQueue *queue, Error **errp);
 void bdrv_reopen_commit(BDRVReopenState *reopen_state);
-- 
2.11.0

[Qemu-devel] [PATCH v5 02/16] block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 4e82c71ba4..cfa53f7114 100644
--- a/block.c
+++ b/block.c
@@ -1079,11 +1079,11 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
                                         const char *filename, Error **errp)
 {
     BlockDriverState *parent = c->opaque;
-    int orig_flags = bdrv_get_flags(parent);
+    bool read_only = bdrv_is_read_only(parent);
     int ret;
 
-    if (!(orig_flags & BDRV_O_RDWR)) {
-        ret = bdrv_reopen(parent, orig_flags | BDRV_O_RDWR, errp);
+    if (read_only) {
+        ret = bdrv_reopen_set_read_only(parent, false, errp);
         if (ret < 0) {
             return ret;
         }
@@ -1095,8 +1095,8 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
         error_setg_errno(errp, -ret, "Could not update backing file link");
     }
 
-    if (!(orig_flags & BDRV_O_RDWR)) {
-        bdrv_reopen(parent, orig_flags, NULL);
+    if (read_only) {
+        bdrv_reopen_set_read_only(parent, true, NULL);
     }
 
     return ret;
-- 
2.11.0

[Qemu-devel] [PATCH v5 03/16] block: Use bdrv_reopen_set_read_only() in commit_start/complete()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/commit.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index a2da5740b0..a53c2d04b0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -38,7 +38,7 @@ typedef struct CommitBlockJob {
     BlockBackend *base;
     BlockDriverState *base_bs;
     BlockdevOnError on_error;
-    int base_flags;
+    bool base_read_only;
     char *backing_file_str;
 } CommitBlockJob;
 
@@ -124,8 +124,8 @@ static void commit_clean(Job *job)
     /* restore base open flags here if appropriate (e.g., change the base back
      * to r/o). These reopens do not need to be atomic, since we won't abort
      * even on failure here */
-    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
-        bdrv_reopen(s->base_bs, s->base_flags, NULL);
+    if (s->base_read_only) {
+        bdrv_reopen_set_read_only(s->base_bs, true, NULL);
     }
 
     g_free(s->backing_file_str);
@@ -264,7 +264,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
                   const char *filter_node_name, Error **errp)
 {
     CommitBlockJob *s;
-    int orig_base_flags;
     BlockDriverState *iter;
     BlockDriverState *commit_top_bs = NULL;
     Error *local_err = NULL;
@@ -283,11 +282,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     }
 
     /* convert base to r/w, if necessary */
-    orig_base_flags = bdrv_get_flags(base);
-    if (!(orig_base_flags & BDRV_O_RDWR)) {
-        bdrv_reopen(base, orig_base_flags | BDRV_O_RDWR, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
+    s->base_read_only = bdrv_is_read_only(base);
+    if (s->base_read_only) {
+        if (bdrv_reopen_set_read_only(base, false, errp) != 0) {
             goto fail;
         }
     }
@@ -363,7 +360,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
-    s->base_flags = orig_base_flags;
     s->backing_file_str = g_strdup(backing_file_str);
     s->on_error = on_error;
 
-- 
2.11.0

[Qemu-devel] [PATCH v5 04/16] block: Use bdrv_reopen_set_read_only() in bdrv_commit()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/commit.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index a53c2d04b0..53148e610b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -391,7 +391,7 @@ int bdrv_commit(BlockDriverState *bs)
     BlockDriverState *commit_top_bs = NULL;
     BlockDriver *drv = bs->drv;
     int64_t offset, length, backing_length;
-    int ro, open_flags;
+    int ro;
     int64_t n;
     int ret = 0;
     uint8_t *buf = NULL;
@@ -410,10 +410,9 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     ro = bs->backing->bs->read_only;
-    open_flags =  bs->backing->bs->open_flags;
 
     if (ro) {
-        if (bdrv_reopen(bs->backing->bs, open_flags | BDRV_O_RDWR, NULL)) {
+        if (bdrv_reopen_set_read_only(bs->backing->bs, false, NULL)) {
             return -EACCES;
         }
     }
@@ -523,7 +522,7 @@ ro_cleanup:
 
     if (ro) {
         /* ignoring error return here */
-        bdrv_reopen(bs->backing->bs, open_flags & ~BDRV_O_RDWR, NULL);
+        bdrv_reopen_set_read_only(bs->backing->bs, true, NULL);
     }
 
     return ret;
-- 
2.11.0

[Qemu-devel] [PATCH v5 05/16] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/stream.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 81a7ec8ece..262d280ccd 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,7 @@ typedef struct StreamBlockJob {
     BlockDriverState *base;
     BlockdevOnError on_error;
     char *backing_file_str;
-    int bs_flags;
+    bool bs_read_only;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -89,10 +89,10 @@ static void stream_clean(Job *job)
     BlockDriverState *bs = blk_bs(bjob->blk);
 
     /* Reopen the image back in read-only mode if necessary */
-    if (s->bs_flags != bdrv_get_flags(bs)) {
+    if (s->bs_read_only) {
         /* Give up write permissions before making it read-only */
         blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
-        bdrv_reopen(bs, s->bs_flags, NULL);
+        bdrv_reopen_set_read_only(bs, true, NULL);
     }
 
     g_free(s->backing_file_str);
@@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 {
     StreamBlockJob *s;
     BlockDriverState *iter;
-    int orig_bs_flags;
+    int bs_read_only;
 
     /* Make sure that the image is opened in read-write mode */
-    orig_bs_flags = bdrv_get_flags(bs);
-    if (!(orig_bs_flags & BDRV_O_RDWR)) {
-        if (bdrv_reopen(bs, orig_bs_flags | BDRV_O_RDWR, errp) != 0) {
+    bs_read_only = bdrv_is_read_only(bs);
+    if (bs_read_only) {
+        if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
             return;
         }
     }
@@ -261,7 +261,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 
     s->base = base;
     s->backing_file_str = g_strdup(backing_file_str);
-    s->bs_flags = orig_bs_flags;
+    s->bs_read_only = bs_read_only;
 
     s->on_error = on_error;
     trace_stream_start(bs, base, s);
@@ -269,7 +269,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     return;
 
 fail:
-    if (orig_bs_flags != bdrv_get_flags(bs)) {
-        bdrv_reopen(bs, orig_bs_flags, NULL);
+    if (bs_read_only) {
+        bdrv_reopen_set_read_only(bs, true, NULL);
     }
 }
-- 
2.11.0

Re: [Qemu-devel] [PATCH v5 05/16] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Kevin Wolf]

Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/stream.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 81a7ec8ece..262d280ccd 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -34,7 +34,7 @@ typedef struct StreamBlockJob {
>      BlockDriverState *base;
>      BlockdevOnError on_error;
>      char *backing_file_str;
> -    int bs_flags;
> +    bool bs_read_only;
>  } StreamBlockJob;
>  
>  static int coroutine_fn stream_populate(BlockBackend *blk,
> @@ -89,10 +89,10 @@ static void stream_clean(Job *job)
>      BlockDriverState *bs = blk_bs(bjob->blk);
>  
>      /* Reopen the image back in read-only mode if necessary */
> -    if (s->bs_flags != bdrv_get_flags(bs)) {
> +    if (s->bs_read_only) {
>          /* Give up write permissions before making it read-only */
>          blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
> -        bdrv_reopen(bs, s->bs_flags, NULL);
> +        bdrv_reopen_set_read_only(bs, true, NULL);
>      }
>  
>      g_free(s->backing_file_str);
> @@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>  {
>      StreamBlockJob *s;
>      BlockDriverState *iter;
> -    int orig_bs_flags;
> +    int bs_read_only;

bool certainly?

Kevin

Re: [Qemu-devel] [PATCH v5 05/16] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Alberto Garcia]

On Tue 20 Nov 2018 07:00:29 PM CET, Kevin Wolf wrote:
>> @@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>>  {
>>      StreamBlockJob *s;
>>      BlockDriverState *iter;
>> -    int orig_bs_flags;
>> +    int bs_read_only;
>
> bool certainly?

Oops!

Berto

[Qemu-devel] [PATCH v5 06/16] block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()

[Alberto Garcia]

This patch replaces the bdrv_reopen() calls that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e5b5eb46e2..20d1d84bab 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4100,7 +4100,6 @@ void qmp_change_backing_file(const char *device,
     BlockDriverState *image_bs = NULL;
     Error *local_err = NULL;
     bool ro;
-    int open_flags;
     int ret;
 
     bs = qmp_get_root_bs(device, errp);
@@ -4142,13 +4141,10 @@ void qmp_change_backing_file(const char *device,
     }
 
     /* if not r/w, reopen to make r/w */
-    open_flags = image_bs->open_flags;
     ro = bdrv_is_read_only(image_bs);
 
     if (ro) {
-        bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (bdrv_reopen_set_read_only(image_bs, false, errp) != 0) {
             goto out;
         }
     }
@@ -4164,7 +4160,7 @@ void qmp_change_backing_file(const char *device,
     }
 
     if (ro) {
-        bdrv_reopen(image_bs, open_flags, &local_err);
+        bdrv_reopen_set_read_only(image_bs, true, &local_err);
         error_propagate(errp, local_err);
     }
 
-- 
2.11.0

[Qemu-devel] [PATCH v5 07/16] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()

[Alberto Garcia]

This patch replaces the bdrv_reopen() call that set and remove the
BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 20d1d84bab..112bc62ae5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1703,8 +1703,7 @@ static void external_snapshot_commit(BlkActionState *common)
      * bdrv_reopen_multiple() across all the entries at once, because we
      * don't want to abort all of them if one of them fails the reopen */
     if (!atomic_read(&state->old_bs->copy_on_read)) {
-        bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
-                    NULL);
+        bdrv_reopen_set_read_only(state->old_bs, true, NULL);
     }
 
     aio_context_release(aio_context);
-- 
2.11.0

[Qemu-devel] [PATCH v5 08/16] block: Use bdrv_reopen_set_read_only() in the mirror driver

[Alberto Garcia]

The 'block-commit' QMP command is implemented internally using two
different drivers. If the source image is the active layer then the
mirror driver is used (commit_active_start()), otherwise the commit
driver is used (commit_start()).

In both cases the destination image must be put temporarily in
read-write mode. This is done correctly in the latter case, but what
commit_active_start() does is copy all flags instead.

This patch replaces the bdrv_reopen() calls in that function with
bdrv_reopen_set_read_only() so that only the read-only status is
changed.

A similar change is made in mirror_exit(), which is also used by the
'drive-mirror' and 'blockdev-mirror' commands.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 56d9ef7474..41b6cbaad6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -672,9 +672,10 @@ static int mirror_exit_common(Job *job)
 
     if (s->should_complete && !abort) {
         BlockDriverState *to_replace = s->to_replace ?: src;
+        bool ro = bdrv_is_read_only(to_replace);
 
-        if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
-            bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
+        if (ro != bdrv_is_read_only(target_bs)) {
+            bdrv_reopen_set_read_only(target_bs, ro, NULL);
         }
 
         /* The mirror job has no requests in flight any more, but we need to
@@ -1692,13 +1693,15 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
                          BlockCompletionFunc *cb, void *opaque,
                          bool auto_complete, Error **errp)
 {
-    int orig_base_flags;
+    bool base_read_only;
     Error *local_err = NULL;
 
-    orig_base_flags = bdrv_get_flags(base);
+    base_read_only = bdrv_is_read_only(base);
 
-    if (bdrv_reopen(base, bs->open_flags, errp)) {
-        return;
+    if (base_read_only) {
+        if (bdrv_reopen_set_read_only(base, false, errp) < 0) {
+            return;
+        }
     }
 
     mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
@@ -1717,6 +1720,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
 error_restore_flags:
     /* ignore error and errp for bdrv_reopen, because we want to propagate
      * the original error */
-    bdrv_reopen(base, orig_base_flags, NULL);
+    if (base_read_only) {
+        bdrv_reopen_set_read_only(base, true, NULL);
+    }
     return;
 }
-- 
2.11.0

[Qemu-devel] [PATCH v5 09/16] block: Drop bdrv_reopen()

[Alberto Garcia]

No one is using this function anymore, so we can safely remove it.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 21 ---------------------
 include/block/block.h |  1 -
 2 files changed, 22 deletions(-)

diff --git a/block.c b/block.c
index cfa53f7114..31de7b2299 100644
--- a/block.c
+++ b/block.c
@@ -3106,27 +3106,6 @@ cleanup:
     return ret;
 }
 
-
-/* Reopen a single BlockDriverState with the specified flags. */
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
-{
-    int ret = -1;
-    Error *local_err = NULL;
-    BlockReopenQueue *queue;
-
-    bdrv_subtree_drained_begin(bs);
-
-    queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
-    ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-    }
-
-    bdrv_subtree_drained_end(bs);
-
-    return ret;
-}
-
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 382e6643fc..de72c7a093 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -302,7 +302,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs,
                                     QDict *options, int flags);
 int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp);
-int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
-- 
2.11.0

[Qemu-devel] [PATCH v5 10/16] qemu-io: Put flag changes in the options QDict in reopen_f()

[Alberto Garcia]

When reopen_f() puts a block device in the reopen queue, some of the
new options are passed using a QDict, but others ("read-only" and the
cache options) are passed as flags.

This patch puts those flags in the QDict. This way the flags parameter
becomes redundant and we'll be able to get rid of it in a subsequent
patch.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qemu-io-cmds.c             | 27 ++++++++++++++++++++++++++-
 tests/qemu-iotests/133     |  9 +++++++++
 tests/qemu-iotests/133.out |  8 ++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5363482213..c9ae09d574 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -10,6 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu-io.h"
 #include "sysemu/block-backend.h"
 #include "block/block.h"
@@ -1978,6 +1979,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
     int flags = bs->open_flags;
     bool writethrough = !blk_enable_write_cache(blk);
     bool has_rw_option = false;
+    bool has_cache_option = false;
 
     BlockReopenQueue *brq;
     Error *local_err = NULL;
@@ -1989,6 +1991,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
                 error_report("Invalid cache option: %s", optarg);
                 return -EINVAL;
             }
+            has_cache_option = true;
             break;
         case 'o':
             if (!qemu_opts_parse_noisily(&reopen_opts, optarg, 0)) {
@@ -2046,9 +2049,31 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
     }
 
     qopts = qemu_opts_find(&reopen_opts, NULL);
-    opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+    opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
     qemu_opts_reset(&reopen_opts);
 
+    if (qdict_haskey(opts, BDRV_OPT_READ_ONLY)) {
+        if (has_rw_option) {
+            error_report("Cannot set both -r/-w and '" BDRV_OPT_READ_ONLY "'");
+            qobject_unref(opts);
+            return -EINVAL;
+        }
+    } else {
+        qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR));
+    }
+
+    if (qdict_haskey(opts, BDRV_OPT_CACHE_DIRECT) ||
+        qdict_haskey(opts, BDRV_OPT_CACHE_NO_FLUSH)) {
+        if (has_cache_option) {
+            error_report("Cannot set both -c and the cache options");
+            qobject_unref(opts);
+            return -EINVAL;
+        }
+    } else {
+        qdict_put_bool(opts, BDRV_OPT_CACHE_DIRECT, flags & BDRV_O_NOCACHE);
+        qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
+    }
+
     bdrv_subtree_drained_begin(bs);
     brq = bdrv_reopen_queue(NULL, bs, opts, flags);
     bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index af6b3e1dd4..14e6b3b972 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -92,6 +92,15 @@ echo
 IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \
     "json:{'driver': 'null-co', 'size': 65536}"
 
+echo
+echo "=== Check that mixing -c/-r/-w and their corresponding options is forbidden ==="
+echo
+
+$QEMU_IO -c 'reopen -r -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
+$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index f4a85aeb63..48a9d087f0 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -24,4 +24,12 @@ Cannot change the option 'driver'
 
 format name: null-co
 format name: null-co
+
+=== Check that mixing -c/-r/-w and their corresponding options is forbidden ===
+
+Cannot set both -r/-w and 'read-only'
+Cannot set both -r/-w and 'read-only'
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
+Cannot set both -c and the cache options
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v5 11/16] block: Clean up reopen_backing_file() in block/replication.c

[Alberto Garcia]

This function is used to put the hidden and secondary disks in
read-write mode before launching the backup job, and back in read-only
mode afterwards.

This patch does the following changes:

  - Use an options QDict with the "read-only" option instead of
    passing the changes as flags only.

  - Simplify the code (it was unnecessarily complicated and verbose).

  - Fix a bug due to which the secondary disk was not being put back
    in read-only mode when writable=false (because in this case
    orig_secondary_flags always had the BDRV_O_RDWR flag set).

  - Stop clearing the BDRV_O_INACTIVE flag.

The flags parameter to bdrv_reopen_queue() becomes redundant and we'll
be able to get rid of it in a subsequent patch.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/replication.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 6349d6958e..481a225924 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -20,6 +20,7 @@
 #include "block/block_backup.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "replication.h"
 
 typedef enum {
@@ -39,8 +40,8 @@ typedef struct BDRVReplicationState {
     char *top_id;
     ReplicationState *rs;
     Error *blocker;
-    int orig_hidden_flags;
-    int orig_secondary_flags;
+    bool orig_hidden_read_only;
+    bool orig_secondary_read_only;
     int error;
 } BDRVReplicationState;
 
@@ -371,44 +372,40 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
     }
 }
 
+/* This function is supposed to be called twice:
+ * first with writable = true, then with writable = false.
+ * The first call puts s->hidden_disk and s->secondary_disk in
+ * r/w mode, and the second puts them back in their original state.
+ */
 static void reopen_backing_file(BlockDriverState *bs, bool writable,
                                 Error **errp)
 {
     BDRVReplicationState *s = bs->opaque;
     BlockReopenQueue *reopen_queue = NULL;
-    int orig_hidden_flags, orig_secondary_flags;
-    int new_hidden_flags, new_secondary_flags;
     Error *local_err = NULL;
 
     if (writable) {
-        orig_hidden_flags = s->orig_hidden_flags =
-                                bdrv_get_flags(s->hidden_disk->bs);
-        new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) &
-                                                    ~BDRV_O_INACTIVE;
-        orig_secondary_flags = s->orig_secondary_flags =
-                                bdrv_get_flags(s->secondary_disk->bs);
-        new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) &
-                                                     ~BDRV_O_INACTIVE;
-    } else {
-        orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) &
-                                                    ~BDRV_O_INACTIVE;
-        new_hidden_flags = s->orig_hidden_flags;
-        orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) &
-                                                    ~BDRV_O_INACTIVE;
-        new_secondary_flags = s->orig_secondary_flags;
+        s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
+        s->orig_secondary_read_only = bdrv_is_read_only(s->secondary_disk->bs);
     }
 
     bdrv_subtree_drained_begin(s->hidden_disk->bs);
     bdrv_subtree_drained_begin(s->secondary_disk->bs);
 
-    if (orig_hidden_flags != new_hidden_flags) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, NULL,
-                                         new_hidden_flags);
+    if (s->orig_hidden_read_only) {
+        int flags = bdrv_get_flags(s->hidden_disk->bs);
+        QDict *opts = qdict_new();
+        qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
+        reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
+                                         opts, flags);
     }
 
-    if (!(orig_secondary_flags & BDRV_O_RDWR)) {
+    if (s->orig_secondary_read_only) {
+        int flags = bdrv_get_flags(s->secondary_disk->bs);
+        QDict *opts = qdict_new();
+        qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
         reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
-                                         NULL, new_secondary_flags);
+                                         opts, flags);
     }
 
     if (reopen_queue) {
-- 
2.11.0

[Qemu-devel] [PATCH v5 12/16] block: Remove flags parameter from bdrv_reopen_queue()

[Alberto Garcia]

Now that all callers are passing all flag changes as QDict options,
the flags parameter is no longer necessary, so we can get rid of it.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 5 +++--
 block/replication.c   | 6 ++----
 include/block/block.h | 3 +--
 qemu-io-cmds.c        | 2 +-
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 31de7b2299..8b401e1cf4 100644
--- a/block.c
+++ b/block.c
@@ -3041,8 +3041,9 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs,
-                                    QDict *options, int flags)
+                                    QDict *options)
 {
+    int flags = bdrv_get_flags(bs);
     return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
                                    NULL, NULL, 0);
 }
@@ -3116,7 +3117,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
     qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
 
     bdrv_subtree_drained_begin(bs);
-    queue = bdrv_reopen_queue(NULL, bs, opts, bdrv_get_flags(bs));
+    queue = bdrv_reopen_queue(NULL, bs, opts);
     ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
     bdrv_subtree_drained_end(bs);
 
diff --git a/block/replication.c b/block/replication.c
index 481a225924..fdbfe47fa4 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -393,19 +393,17 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
     bdrv_subtree_drained_begin(s->secondary_disk->bs);
 
     if (s->orig_hidden_read_only) {
-        int flags = bdrv_get_flags(s->hidden_disk->bs);
         QDict *opts = qdict_new();
         qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
         reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
-                                         opts, flags);
+                                         opts);
     }
 
     if (s->orig_secondary_read_only) {
-        int flags = bdrv_get_flags(s->secondary_disk->bs);
         QDict *opts = qdict_new();
         qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
         reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
-                                         opts, flags);
+                                         opts);
     }
 
     if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index de72c7a093..f70a843b72 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -299,8 +299,7 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
 BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
                                        int flags, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-                                    BlockDriverState *bs,
-                                    QDict *options, int flags);
+                                    BlockDriverState *bs, QDict *options);
 int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **errp);
 int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index c9ae09d574..2c39124036 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2075,7 +2075,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
     }
 
     bdrv_subtree_drained_begin(bs);
-    brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+    brq = bdrv_reopen_queue(NULL, bs, opts);
     bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
     bdrv_subtree_drained_end(bs);
 
-- 
2.11.0

[Qemu-devel] [PATCH v5 13/16] block: Stop passing flags to bdrv_reopen_queue_child()

[Alberto Garcia]

Now that all callers are passing the new options using the QDict we no
longer need the 'flags' parameter.

This patch makes the following changes:

   1) The update_options_from_flags() call is no longer necessary
      so it can be removed.

   2) The update_flags_from_options() call is now used in all cases,
      and is moved down a few lines so it happens after the options
      QDict contains the final set of values.

   3) The flags parameter is removed. Now the flags are initialized
      using the current value (for the top-level node) or the parent
      flags (after inherit_options()). In both cases the initial
      values are updated to reflect the new options in the QDict. This
      happens in bdrv_reopen_queue_child() (as explained above) and in
      bdrv_reopen_prepare().

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index 8b401e1cf4..8bc808d6f3 100644
--- a/block.c
+++ b/block.c
@@ -2912,7 +2912,6 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
 static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
                                                  BlockDriverState *bs,
                                                  QDict *options,
-                                                 int flags,
                                                  const BdrvChildRole *role,
                                                  QDict *parent_options,
                                                  int parent_flags)
@@ -2921,7 +2920,9 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
     BlockReopenQueueEntry *bs_entry;
     BdrvChild *child;
-    QDict *old_options, *explicit_options;
+    QDict *old_options, *explicit_options, *options_copy;
+    int flags;
+    QemuOpts *opts;
 
     /* Make sure that the caller remembered to use a drained section. This is
      * important to avoid graph changes between the recursive queuing here and
@@ -2947,22 +2948,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     /*
      * Precedence of options:
      * 1. Explicitly passed in options (highest)
-     * 2. Set in flags (only for top level)
-     * 3. Retained from explicitly set options of bs
-     * 4. Inherited from parent node
-     * 5. Retained from effective options of bs
+     * 2. Retained from explicitly set options of bs
+     * 3. Inherited from parent node
+     * 4. Retained from effective options of bs
      */
 
-    if (!parent_options) {
-        /*
-         * Any setting represented by flags is always updated. If the
-         * corresponding QDict option is set, it takes precedence. Otherwise
-         * the flag is translated into a QDict option. The old setting of bs is
-         * not considered.
-         */
-        update_options_from_flags(options, flags);
-    }
-
     /* Old explicitly set values (don't overwrite by inherited value) */
     if (bs_entry) {
         old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
@@ -2976,16 +2966,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
     /* Inherit from parent node */
     if (parent_options) {
-        QemuOpts *opts;
-        QDict *options_copy;
-        assert(!flags);
+        flags = 0;
         role->inherit_options(&flags, options, parent_flags, parent_options);
-        options_copy = qdict_clone_shallow(options);
-        opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
-        qemu_opts_absorb_qdict(opts, options_copy, NULL);
-        update_flags_from_options(&flags, opts);
-        qemu_opts_del(opts);
-        qobject_unref(options_copy);
+    } else {
+        flags = bdrv_get_flags(bs);
     }
 
     /* Old values are used for options that aren't set yet */
@@ -2993,6 +2977,14 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     bdrv_join_options(bs, options, old_options);
     qobject_unref(old_options);
 
+    /* We have the final set of options so let's update the flags */
+    options_copy = qdict_clone_shallow(options);
+    opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options_copy, NULL);
+    update_flags_from_options(&flags, opts);
+    qemu_opts_del(opts);
+    qobject_unref(options_copy);
+
     /* bdrv_open_inherit() sets and clears some additional flags internally */
     flags &= ~BDRV_O_PROTOCOL;
     if (flags & BDRV_O_RDWR) {
@@ -3032,7 +3024,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
         qdict_extract_subqdict(options, &new_child_options, child_key_dot);
         g_free(child_key_dot);
 
-        bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
+        bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
                                 child->role, options, flags);
     }
 
@@ -3043,9 +3035,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs,
                                     QDict *options)
 {
-    int flags = bdrv_get_flags(bs);
-    return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
-                                   NULL, NULL, 0);
+    return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, NULL, 0);
 }
 
 /*
-- 
2.11.0

[Qemu-devel] [PATCH v5 14/16] block: Remove assertions from update_flags_from_options()

[Alberto Garcia]

This function takes four options (cache.direct, cache.no-flush,
read-only and auto-read-only) from a QemuOpts object and updates the
flags accordingly.

If any of those options is not set (because it was missing from the
original QDict or because it had an invalid value) then the function
aborts with a failed assertion:

   $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
   block.c:1126: update_flags_from_options: Assertion `qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)' failed.
   Aborted

This assertion is unnecessary, and it forces any caller of
bdrv_reopen() to pass all the aforementioned four options. This may
have made sense in order to remove ambiguity when bdrv_reopen() was
taking both flags and options, but that's not the case anymore.

It's also unnecessary if we want to validate the option values,
because bdrv_reopen_prepare() already takes care of that, as we can
see if we remove the assertions:

   $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
   Parameter 'read-only' expects 'on' or 'off'

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    | 4 ----
 tests/qemu-iotests/133     | 9 +++++++++
 tests/qemu-iotests/133.out | 7 +++++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 8bc808d6f3..68f1e3b45e 100644
--- a/block.c
+++ b/block.c
@@ -1139,24 +1139,20 @@ static void update_flags_from_options(int *flags, QemuOpts *opts)
 {
     *flags &= ~BDRV_O_CACHE_MASK;
 
-    assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
     if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
         *flags |= BDRV_O_NO_FLUSH;
     }
 
-    assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
     if (qemu_opt_get_bool_del(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_del(opts, BDRV_OPT_READ_ONLY, false)) {
         *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;
     }
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index 14e6b3b972..243b1eba50 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -101,6 +101,15 @@ $QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
 $QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
 $QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
 $QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
+
+echo
+echo "=== Check that invalid options are handled correctly ==="
+echo
+
+$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
+$QEMU_IO -c 'reopen -o cache.no-flush=bar' $TEST_IMG
+$QEMU_IO -c 'reopen -o cache.direct=baz' $TEST_IMG
+$QEMU_IO -c 'reopen -o auto-read-only=qux' $TEST_IMG
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index 48a9d087f0..414c7fa27f 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -32,4 +32,11 @@ Cannot set both -r/-w and 'read-only'
 Cannot set both -c and the cache options
 Cannot set both -c and the cache options
 Cannot set both -c and the cache options
+
+=== Check that invalid options are handled correctly ===
+
+Parameter 'read-only' expects 'on' or 'off'
+Parameter 'cache.no-flush' expects 'on' or 'off'
+Parameter 'cache.direct' expects 'on' or 'off'
+Parameter 'auto-read-only' expects 'on' or 'off'
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v5 15/16] block: Assert that flags are up-to-date in bdrv_reopen_prepare()

[Alberto Garcia]

Towards the end of bdrv_reopen_queue_child(), before starting to
process the children, the update_flags_from_options() function is
called in order to have BDRVReopenState.flags in sync with the options
from the QDict.

This is necessary because during the reopen process flags must be
updated for all nodes in the queue so bdrv_is_writable_after_reopen()
and the permission checks work correctly.

Because of that, calling update_flags_from_options() again in
bdrv_reopen_prepare() doesn't really change the flags (they are
already up-to-date). But we need to call it in order to remove those
options from QemuOpts and that way indicate that they have been
processed.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block.c b/block.c
index 68f1e3b45e..03277b3d19 100644
--- a/block.c
+++ b/block.c
@@ -3178,6 +3178,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
                         Error **errp)
 {
     int ret = -1;
+    int old_flags;
     Error *local_err = NULL;
     BlockDriver *drv;
     QemuOpts *opts;
@@ -3203,7 +3204,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    /* This was already called in bdrv_reopen_queue_child() so the flags
+     * are up-to-date. This time we simply want to remove the options from
+     * QemuOpts in order to indicate that they have been processed. */
+    old_flags = reopen_state->flags;
     update_flags_from_options(&reopen_state->flags, opts);
+    assert(old_flags == reopen_state->flags);
 
     discard = qemu_opt_get_del(opts, BDRV_OPT_DISCARD);
     if (discard != NULL) {
-- 
2.11.0

[Qemu-devel] [PATCH v5 16/16] block: Fix update of BDRV_O_AUTO_RDONLY in update_flags_from_options()

[Alberto Garcia]

Commit e35bdc123a4ace9f4d3fcca added the auto-read-only option and the
code to update its corresponding flag in update_flags_from_options(),
but forgot to clear the flag if auto-read-only is false.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reported-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 03277b3d19..e1a969e331 100644
--- a/block.c
+++ b/block.c
@@ -1137,7 +1137,7 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
 
 static void update_flags_from_options(int *flags, QemuOpts *opts)
 {
-    *flags &= ~BDRV_O_CACHE_MASK;
+    *flags &= ~(BDRV_O_CACHE_MASK | BDRV_O_RDWR | BDRV_O_AUTO_RDONLY);
 
     if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
         *flags |= BDRV_O_NO_FLUSH;
@@ -1147,8 +1147,6 @@ static void update_flags_from_options(int *flags, QemuOpts *opts)
         *flags |= BDRV_O_NOCACHE;
     }
 
-    *flags &= ~BDRV_O_RDWR;
-
     if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) {
         *flags |= BDRV_O_RDWR;
     }
-- 
2.11.0

Re: [Qemu-devel] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Kevin Wolf]

Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
> Hi all,
> 
> when reopening a BlockDriverState using bdrv_reopen() and friends the
> new options can be specified either with a QDict or with flags. Both
> methods overlap and that makes the semantics and the implementation
> unnecessarily complicated.
> 
> This series removes the 'flags' parameter from these functions, so
> from now on all option changes must be specified using a QDict. Apart
> from simplifying the API, a few bugs are fixed along the way. See the
> individual patches for more details.
> 
> This was tested with the current master (460f0236c12a86a38692c12d9bf).

Looks good to me, except for that one s/int/bool/ that I could do while
applying.

The only remaining question is - is all of this for 3.1, or which parts
are? It's a long series, but there are also a few bug fixes in there.

Kevin

Re: [Qemu-devel] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Alberto Garcia]

On Tue 20 Nov 2018 07:21:21 PM CET, Kevin Wolf wrote:
> Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
>> Hi all,
>> 
>> when reopening a BlockDriverState using bdrv_reopen() and friends the
>> new options can be specified either with a QDict or with flags. Both
>> methods overlap and that makes the semantics and the implementation
>> unnecessarily complicated.
>> 
>> This series removes the 'flags' parameter from these functions, so
>> from now on all option changes must be specified using a QDict. Apart
>> from simplifying the API, a few bugs are fixed along the way. See the
>> individual patches for more details.
>> 
>> This was tested with the current master (460f0236c12a86a38692c12d9bf).
>
> Looks good to me, except for that one s/int/bool/ that I could do while
> applying.
>
> The only remaining question is - is all of this for 3.1, or which parts
> are? It's a long series, but there are also a few bug fixes in there.

It wasn't meant for 3.1, I don't think there's any urgency for that.
Patch 16 is perhaps worth applying now (and even patch 14), but the rest
can wait.

Berto

Re: [Qemu-devel] [PATCH v5 00/16] Don't pass flags to bdrv_reopen_queue()

[Kevin Wolf]

Am 12.11.2018 um 15:00 hat Alberto Garcia geschrieben:
> Hi all,
> 
> when reopening a BlockDriverState using bdrv_reopen() and friends the
> new options can be specified either with a QDict or with flags. Both
> methods overlap and that makes the semantics and the implementation
> unnecessarily complicated.
> 
> This series removes the 'flags' parameter from these functions, so
> from now on all option changes must be specified using a QDict. Apart
> from simplifying the API, a few bugs are fixed along the way. See the
> individual patches for more details.
> 
> This was tested with the current master (460f0236c12a86a38692c12d9bf).

Thanks, applied patch 16 to block and the rest of the series to
block-next.

Kevin