[PATCH 0/2] block/qcow2: Fix bitmap reopen with 'auto-read-only' file child

[PATCH 0/2] block/qcow2: Fix bitmap reopen with 'auto-read-only' file child

[Peter Krempa]

See patch 2/2 for explanation. Also please excuse the lack of tests
caused by my ignorance of not figuring out where to put them.

Peter Krempa (2):
  block: Introduce 'bdrv_reopen_commit_post' step
  block/qcow2: Move bitmap reopen into bdrv_reopen_commit_post

 block.c                   | 9 +++++++++
 block/qcow2.c             | 7 ++++++-
 include/block/block_int.h | 1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.24.1

[PATCH 1/2] block: Introduce 'bdrv_reopen_commit_post' step

[Peter Krempa]

Add another step in the reopen process where driver can execute code
after permission changes are comitted.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 block.c                   | 9 +++++++++
 include/block/block_int.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/block.c b/block.c
index 1bdb9c679d..a82e53d4cc 100644
--- a/block.c
+++ b/block.c
@@ -3694,6 +3694,15 @@ cleanup_perm:
             }
         }
     }
+
+    if (ret == 0) {
+        QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
+            BlockDriverState *bs = bs_entry->state.bs;
+
+            if (bs->drv->bdrv_reopen_commit_post)
+                bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
+        }
+    }
 cleanup:
     QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
         if (ret) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6f9fd5e20e..f422c0bff0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -122,6 +122,7 @@ struct BlockDriver {
     int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state,
                                BlockReopenQueue *queue, Error **errp);
     void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
+    void (*bdrv_reopen_commit_post)(BDRVReopenState *reopen_state);
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
     void (*bdrv_join_options)(QDict *options, QDict *old_options);

-- 
2.24.1

[PATCH 2/2] block/qcow2: Move bitmap reopen into bdrv_reopen_commit_post

[Peter Krempa]

The bitmap code requires writing the 'file' child when the qcow2 driver
is reopened in read-write mode.

If the 'file' child is being reopened due to a permissions change, the
modification is commited yet when qcow2_reopen_commit is called. This
means that any attempt to write the 'file' child will end with EBADFD
as the original fd was already closed.

Moving bitmap reopening to the new callback which is called after
permission modifications are commited fixes this as the file descriptor
will be replaced with the correct one.

The above problem manifests itself when reopening 'qcow2' format layer
which uses a 'file-posix' file child which was opened with the
'auto-read-only' property set.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 block/qcow2.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..3640e8c07d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1884,6 +1884,11 @@ fail:
 static void qcow2_reopen_commit(BDRVReopenState *state)
 {
     qcow2_update_options_commit(state->bs, state->opaque);
+    g_free(state->opaque);
+}
+
+static void qcow2_reopen_commit_post(BDRVReopenState *state)
+{
     if (state->flags & BDRV_O_RDWR) {
         Error *local_err = NULL;

@@ -1898,7 +1903,6 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
                               bdrv_get_node_name(state->bs));
         }
     }
-    g_free(state->opaque);
 }

 static void qcow2_reopen_abort(BDRVReopenState *state)
@@ -5534,6 +5538,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_close         = qcow2_close,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
     .bdrv_reopen_commit   = qcow2_reopen_commit,
+    .bdrv_reopen_commit_post = qcow2_reopen_commit_post,
     .bdrv_reopen_abort    = qcow2_reopen_abort,
     .bdrv_join_options    = qcow2_join_options,
     .bdrv_child_perm      = bdrv_format_default_perms,
-- 
2.24.1

Re: [PATCH 0/2] block/qcow2: Fix bitmap reopen with 'auto-read-only' file child

[Kevin Wolf]

Am 28.02.2020 um 13:44 hat Peter Krempa geschrieben:
> See patch 2/2 for explanation. Also please excuse the lack of tests
> caused by my ignorance of not figuring out where to put them.

Thanks, applied to the block branch.

A test would probably live in test/qemu-iotests/. Test case 245 is
specifically about x-blockdev-reopen, so we could consider putting it
there, or you could just create a new test case.

Kevin