[PATCH for-7.0 0/2] block/vmdk: Fix reopening bs->file

[PATCH for-7.0 0/2] block/vmdk: Fix reopening bs->file

[Hanna Reitz]

Hi,

A couple of months ago I noticed that changing a vmdk node’s file child
through blockdev-reopen would crash qemu.  I started writing a fix at
some point, got distracted, but now here it is.


Hanna Reitz (2):
  block/vmdk: Fix reopening bs->file
  iotests/reopen-file: Test reopening file child

 block/vmdk.c                             | 56 ++++++++++++++-
 tests/qemu-iotests/tests/reopen-file     | 88 ++++++++++++++++++++++++
 tests/qemu-iotests/tests/reopen-file.out |  5 ++
 3 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/tests/reopen-file
 create mode 100644 tests/qemu-iotests/tests/reopen-file.out

-- 
2.35.1

[PATCH for-7.0 1/2] block/vmdk: Fix reopening bs->file

[Hanna Reitz]

VMDK disk data is stored in extents, which may or may not be separate
from bs->file.  VmdkExtent.file points to where they are stored.  Each
that is stored in bs->file will simply reuse the exact pointer value of
bs->file.

(That is why vmdk_free_extents() will unref VmdkExtent.file (e->file)
only if e->file != bs->file.)

Reopen operations can change bs->file (they will replace the whole
BdrvChild object, not just the BDS stored in that BdrvChild), and then
we will need to change all .file pointers of all such VmdkExtents to
point to the new BdrvChild.

In vmdk_reopen_prepare(), we have to check which VmdkExtents are
affected, and in vmdk_reopen_commit(), we can modify them.  We have to
split this because:
- The new BdrvChild is created only after prepare, so we can change
  VmdkExtent.file only in commit
- In commit, there no longer is any (valid) reference to the old
  BdrvChild object, so there would be nothing to compare VmdkExtent.file
  against to see whether it was equal to bs->file before reopening
  (There is BDRVReopenState.old_file_bs, but the old bs->file
  BdrvChild's .bs pointer will be NULL-ed when the new BdrvChild is
  created, and so we cannot compare VmdkExtent.file->bs against
  BDRVReopenState.old_file_bs)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 block/vmdk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 37c0946066..38e5ab3806 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -178,6 +178,10 @@ typedef struct BDRVVmdkState {
     char *create_type;
 } BDRVVmdkState;
 
+typedef struct BDRVVmdkReopenState {
+    bool *extents_using_bs_file;
+} BDRVVmdkReopenState;
+
 typedef struct VmdkMetaData {
     unsigned int l1_index;
     unsigned int l2_index;
@@ -400,15 +404,63 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
     return 1;
 }
 
-/* We have nothing to do for VMDK reopen, stubs just return success */
 static int vmdk_reopen_prepare(BDRVReopenState *state,
                                BlockReopenQueue *queue, Error **errp)
 {
+    BDRVVmdkState *s;
+    BDRVVmdkReopenState *rs;
+    int i;
+
     assert(state != NULL);
     assert(state->bs != NULL);
+    assert(state->opaque == NULL);
+
+    s = state->bs->opaque;
+
+    rs = g_new0(BDRVVmdkReopenState, 1);
+    state->opaque = rs;
+
+    /*
+     * Check whether there are any extents stored in bs->file; if bs->file
+     * changes, we will need to update their .file pointers to follow suit
+     */
+    rs->extents_using_bs_file = g_new(bool, s->num_extents);
+    for (i = 0; i < s->num_extents; i++) {
+        rs->extents_using_bs_file[i] = s->extents[i].file == state->bs->file;
+    }
+
     return 0;
 }
 
+static void vmdk_reopen_clean(BDRVReopenState *state)
+{
+    BDRVVmdkReopenState *rs = state->opaque;
+
+    g_free(rs->extents_using_bs_file);
+    g_free(rs);
+    state->opaque = NULL;
+}
+
+static void vmdk_reopen_commit(BDRVReopenState *state)
+{
+    BDRVVmdkState *s = state->bs->opaque;
+    BDRVVmdkReopenState *rs = state->opaque;
+    int i;
+
+    for (i = 0; i < s->num_extents; i++) {
+        if (rs->extents_using_bs_file[i]) {
+            s->extents[i].file = state->bs->file;
+        }
+    }
+
+    vmdk_reopen_clean(state);
+}
+
+static void vmdk_reopen_abort(BDRVReopenState *state)
+{
+    vmdk_reopen_clean(state);
+}
+
 static int vmdk_parent_open(BlockDriverState *bs)
 {
     char *p_name;
@@ -3072,6 +3124,8 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_open                    = vmdk_open,
     .bdrv_co_check                = vmdk_co_check,
     .bdrv_reopen_prepare          = vmdk_reopen_prepare,
+    .bdrv_reopen_commit           = vmdk_reopen_commit,
+    .bdrv_reopen_abort            = vmdk_reopen_abort,
     .bdrv_child_perm              = bdrv_default_perms,
     .bdrv_co_preadv               = vmdk_co_preadv,
     .bdrv_co_pwritev              = vmdk_co_pwritev,
-- 
2.35.1

[PATCH for-7.0 2/2] iotests/reopen-file: Test reopening file child

[Hanna Reitz]

This should work for all format drivers that support reopening, so test
it.

(This serves as a regression test for HEAD^: This test used to fail for
VMDK before HEAD^.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/tests/reopen-file     | 88 ++++++++++++++++++++++++
 tests/qemu-iotests/tests/reopen-file.out |  5 ++
 2 files changed, 93 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/reopen-file
 create mode 100644 tests/qemu-iotests/tests/reopen-file.out

diff --git a/tests/qemu-iotests/tests/reopen-file b/tests/qemu-iotests/tests/reopen-file
new file mode 100755
index 0000000000..24fd648050
--- /dev/null
+++ b/tests/qemu-iotests/tests/reopen-file
@@ -0,0 +1,88 @@
+#!/usr/bin/env python3
+# group: rw quick
+#
+# Test reopening a format driver's file child
+#
+# Copyright (C) 2022 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import imgfmt, qemu_img_create, QMPTestCase
+
+
+image_size = 1 * 1024 * 1024
+test_img = os.path.join(iotests.test_dir, 'test.img')
+
+
+class TestReopenFile(QMPTestCase):
+    def setUp(self) -> None:
+        assert qemu_img_create('-f', imgfmt, test_img, str(image_size)) == 0
+
+        # Add format driver node ('format') on top of the file ('file'), then
+        # add another raw node ('raw') on top of 'file' so for the reopen we
+        # can just switch from 'file' to 'raw'
+        self.vm = iotests.VM()
+        self.vm.add_blockdev(self.vm.qmp_to_opts({
+            'driver': imgfmt,
+            'node-name': 'format',
+            'file': {
+                'driver': 'file',
+                'node-name': 'file',
+                'filename': test_img
+            }
+        }))
+        self.vm.add_blockdev(self.vm.qmp_to_opts({
+            'driver': 'raw',
+            'node-name': 'raw',
+            'file': 'file'
+        }))
+        self.vm.launch()
+
+    def tearDown(self) -> None:
+        self.vm.shutdown()
+        os.remove(test_img)
+
+        # Check if there was any qemu-io run that failed
+        if 'Pattern verification failed' in self.vm.get_log():
+            print('ERROR: Pattern verification failed:')
+            print(self.vm.get_log())
+            self.fail('qemu-io pattern verification failed')
+
+    def test_reopen_file(self) -> None:
+        result = self.vm.qmp('blockdev-reopen', options=[{
+            'driver': imgfmt,
+            'node-name': 'format',
+            'file': 'raw'
+        }])
+        self.assert_qmp(result, 'return', {})
+
+        # Do some I/O to the image to see whether it still works
+        # (Pattern verification will be checked by tearDown())
+        result = self.vm.qmp('human-monitor-command',
+                             command_line='qemu-io format "write -P 42 0 64k"')
+        self.assert_qmp(result, 'return', '')
+
+        result = self.vm.qmp('human-monitor-command',
+                             command_line='qemu-io format "read -P 42 0 64k"')
+        self.assert_qmp(result, 'return', '')
+
+
+if __name__ == '__main__':
+    # Must support creating images and reopen
+    iotests.main(supported_fmts=['qcow', 'qcow2', 'qed', 'raw', 'vdi', 'vhdx',
+                                 'vmdk', 'vpc'],
+                 supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/reopen-file.out b/tests/qemu-iotests/tests/reopen-file.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/reopen-file.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
-- 
2.35.1

Re: [PATCH for-7.0 0/2] block/vmdk: Fix reopening bs->file

[Kevin Wolf]

Am 14.03.2022 um 17:27 hat Hanna Reitz geschrieben:
> Hi,
> 
> A couple of months ago I noticed that changing a vmdk node’s file child
> through blockdev-reopen would crash qemu.  I started writing a fix at
> some point, got distracted, but now here it is.

Thanks, fixed up a merge conflict with 2882ccf86a9 in the test case and
applied to the block branch.

Kevin