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

diff --git a/block/mirror.c b/block/mirror.c
index 6cc10df5c9..37f8d3ac56 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -663,13 +663,15 @@ static void mirror_exit(Job *job, void *opaque)
     }
 
     if (s->should_complete && data->ret == 0) {
+        bool ro;
         BlockDriverState *to_replace = src;
         if (s->to_replace) {
             to_replace = s->to_replace;
         }
 
-        if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
-            bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
+        ro = bdrv_is_read_only(to_replace);
+        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
@@ -1674,13 +1676,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,
@@ -1699,6 +1703,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

On 19.09.18 16:47, Alberto Garcia wrote:
> 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.

Well, not only commit_active_start().  mirror_exit() does exactly the
same.  It does seem on purpose to let the target have exactly the same
flags as the source eventually.

Then again, none of the other flags seem to make any sense to me here.
Therefore, I tend to agree that it is a bug fix, even though I wouldn't
say it was an oversight.

Reviewed-by: Max Reitz <mreitz@redhat.com>

...eagerly awaiting rebase on 737efc1eda2.

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