The following changes since commit 2e3408b3cc7de4e87a9adafc8c19bfce3abec947:

Merge tag 'misc-pull-request' of gitlab.com:marcandre.lureau/qemu into staging (2022-05-03 09:13:17 -0700)

for you to fetch changes up to c1fe694357a328c807ae3cc6961c19e923448fcc:

coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS() (2022-05-04 15:55:23 +0200)

- Fix and re-enable GLOBAL_STATE_CODE assertions

- vhost-user: Fixes for VHOST_USER_ADD/REM_MEM_REG

- vmdk: Fix reopening bs->file

- coroutine: use QEMU_DEFINE_STATIC_CO_TLS()

- docs/qemu-img: Fix list of formats which implement check

Denis V. Lunev (1):

qemu-img: properly list formats which have consistency check implemented

Hanna Reitz (6):

block: Classify bdrv_get_flags() as I/O function

qcow2: Do not reopen data_file in invalidate_cache

Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"

iotests: Add regression test for issue 945

block/vmdk: Fix reopening bs->file

iotests/reopen-file: Test reopening file child

Kevin Wolf (3):

docs/vhost-user: Clarifications for VHOST_USER_ADD/REM_MEM_REG

libvhost-user: Fix extra vu_add/rem_mem_reg reply

vhost-user: Don't pass file descriptor for VHOST_USER_REM_MEM_REG

Stefan Hajnoczi (3):

coroutine-ucontext: use QEMU_DEFINE_STATIC_CO_TLS()

coroutine: use QEMU_DEFINE_STATIC_CO_TLS()

coroutine-win32: use QEMU_DEFINE_STATIC_CO_TLS()

docs/interop/vhost-user.rst | 17 ++++

docs/tools/qemu-img.rst | 4 +-

include/block/block-global-state.h | 1 -

include/block/block-io.h | 1 +

include/qemu/main-loop.h | 3 +-

block.c | 2 +-

block/qcow2.c | 104 ++++++++++++---------

block/vmdk.c | 56 ++++++++++-

hw/virtio/vhost-user.c | 2 +-

subprojects/libvhost-user/libvhost-user.c | 17 ++--

util/coroutine-ucontext.c | 38 +++++---

util/coroutine-win32.c | 18 +++-

util/qemu-coroutine.c | 41 ++++----

tests/qemu-iotests/tests/export-incoming-iothread | 81 ++++++++++++++++

.../tests/export-incoming-iothread.out | 5 +

tests/qemu-iotests/tests/reopen-file | 89 ++++++++++++++++++

tests/qemu-iotests/tests/reopen-file.out | 5 +

17 files changed, 388 insertions(+), 96 deletions(-)

create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread

create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out

create mode 100755 tests/qemu-iotests/tests/reopen-file

create mode 100644 tests/qemu-iotests/tests/reopen-file.out

From: "Denis V. Lunev" <den@openvz.org>

Simple grep for the .bdrv_co_check callback presence gives the following

list of block drivers

* QED

* VDI

* VHDX

* VMDK

* Parallels

which have this callback. The presense of the callback means that

consistency check is supported.

The patch updates documentation accordingly.

Signed-off-by: Denis V. Lunev <den@openvz.org>

CC: Kevin Wolf <kwolf@redhat.com>

CC: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20220407083932.531965-1-den@openvz.org>

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

docs/tools/qemu-img.rst | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst

index XXXXXXX..XXXXXXX 100644

--- a/docs/tools/qemu-img.rst

+++ b/docs/tools/qemu-img.rst

@@ -XXX,XX +XXX,XX @@ Command description:

``-r all`` fixes all kinds of errors, with a higher risk of choosing the

wrong fix or hiding corruption that has already occurred.

- Only the formats ``qcow2``, ``qed`` and ``vdi`` support

- consistency checks.

+ Only the formats ``qcow2``, ``qed``, ``parallels``, ``vhdx``, ``vmdk`` and

+ ``vdi`` support consistency checks.

In case the image does not have any inconsistencies, check exits with ``0``.

Other exit codes indicate the kind of inconsistency found or if another error

2.35.1

The specification for VHOST_USER_ADD/REM_MEM_REG messages is unclear

in several points, which has led to clients having incompatible

implementations. This changes the specification to be more explicit

about them:

* VHOST_USER_ADD_MEM_REG is not specified as receiving a file

descriptor, though it obviously does need to do so. All

implementations agree on this one, fix the specification.

* VHOST_USER_REM_MEM_REG is not specified as receiving a file

descriptor either, and it also has no reason to do so. rust-vmm does

not send file descriptors for removing a memory region (in agreement

with the specification), libvhost-user and QEMU do (which is a bug),

though libvhost-user doesn't actually make any use of it.

Change the specification so that for compatibility QEMU's behaviour

becomes legal, even if discouraged, but rust-vmm's behaviour becomes

the explicitly recommended mode of operation.

* VHOST_USER_ADD_MEM_REG doesn't have a documented return value, which

is the desired behaviour in the non-postcopy case. It also implemented

like this in QEMU and rust-vmm, though libvhost-user is buggy and

sometimes sends an unexpected reply. This will be fixed in a separate

patch.

However, in postcopy mode it does reply like VHOST_USER_SET_MEM_TABLE.

This behaviour is shared between libvhost-user and QEMU; rust-vmm

doesn't implement postcopy mode yet. Mention it explicitly in the

spec.

* The specification doesn't mention how VHOST_USER_REM_MEM_REG

identifies the memory region to be removed. Change it to describe the

existing behaviour of libvhost-user (guest address, user address and

size must match).

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

Message-Id: <20220407133657.155281-2-kwolf@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

docs/interop/vhost-user.rst | 17 +++++++++++++++++

1 file changed, 17 insertions(+)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst

--- a/docs/interop/vhost-user.rst

+++ b/docs/interop/vhost-user.rst

@@ -XXX,XX +XXX,XX @@ replies. Here is a list of the ones that do:

There are several messages that the master sends with file descriptors passed

in the ancillary data:

+* ``VHOST_USER_ADD_MEM_REG``

* ``VHOST_USER_SET_MEM_TABLE``

* ``VHOST_USER_SET_LOG_BASE`` (if ``VHOST_USER_PROTOCOL_F_LOG_SHMFD``)

* ``VHOST_USER_SET_LOG_FD``

@@ -XXX,XX +XXX,XX @@ Master message types

``VHOST_USER_REM_MEM_REG`` message, this message is used to set and

update the memory tables of the slave device.

+ Exactly one file descriptor from which the memory is mapped is

+ passed in the ancillary data.

+

+ In postcopy mode (see ``VHOST_USER_POSTCOPY_LISTEN``), the slave

+ replies with the bases of the memory mapped region to the master.

+ For further details on postcopy, see ``VHOST_USER_SET_MEM_TABLE``.

+ They apply to ``VHOST_USER_ADD_MEM_REG`` accordingly.

+

``VHOST_USER_REM_MEM_REG``

:id: 38

:equivalent ioctl: N/A

@@ -XXX,XX +XXX,XX @@ Master message types

``VHOST_USER_ADD_MEM_REG`` message, this message is used to set and

update the memory tables of the slave device.

+ The memory region to be removed is identified by its guest address,

+ user address and size. The mmap offset is ignored.

+

+ No file descriptors SHOULD be passed in the ancillary data. For

+ compatibility with existing incorrect implementations, the slave MAY

+ accept messages with one file descriptor. If a file descriptor is

+ passed, the slave MUST close it without using it otherwise.

+

``VHOST_USER_SET_STATUS``

:id: 39

:equivalent ioctl: VHOST_VDPA_SET_STATUS

2.35.1

Outside of postcopy mode, neither VHOST_USER_ADD_MEM_REG nor

VHOST_USER_REM_MEM_REG are supposed to send a reply unless explicitly

requested with the need_reply flag. Their current implementation always

sends a reply, even if it isn't requested. This confuses the master

because it will interpret the reply as a reply for the next message for

which it actually expects a reply.

need_reply is already handled correctly by vu_dispatch(), so just don't

send a reply in the non-postcopy part of the message handler for these

two commands.

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

Message-Id: <20220407133657.155281-3-kwolf@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

subprojects/libvhost-user/libvhost-user.c | 9 +++------

1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c

--- a/subprojects/libvhost-user/libvhost-user.c

+++ b/subprojects/libvhost-user/libvhost-user.c

@@ -XXX,XX +XXX,XX @@ vu_add_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {

DPRINT("Successfully added new region\n");

dev->nregions++;

- vmsg_set_reply_u64(vmsg, 0);

- return true;

+ return false;

}

@@ -XXX,XX +XXX,XX @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {

}

- if (found) {

- vmsg_set_reply_u64(vmsg, 0);

- } else {

+ if (!found) {

vu_panic(dev, "Specified region not found\n");

}

close(vmsg->fds[0]);

- return true;

+ return false;

static bool

2.35.1

The spec clarifies now that QEMU should not send a file descriptor in a

request to remove a memory region. Change it accordingly.

For libvhost-user, this is a bug fix that makes it compatible with

rust-vmm's implementation that doesn't send a file descriptor. Keep

accepting, but ignoring a file descriptor for compatibility with older

QEMU versions.

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

Message-Id: <20220407133657.155281-4-kwolf@redhat.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

hw/virtio/vhost-user.c | 2 +-

subprojects/libvhost-user/libvhost-user.c | 8 ++++----

2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c

--- a/hw/virtio/vhost-user.c

+++ b/hw/virtio/vhost-user.c

@@ -XXX,XX +XXX,XX @@ static int send_remove_regions(struct vhost_dev *dev,

vhost_user_fill_msg_region(&region_buffer, shadow_reg, 0);

msg->payload.mem_reg.region = region_buffer;

- ret = vhost_user_write(dev, msg, &fd, 1);

+ ret = vhost_user_write(dev, msg, NULL, 0);

if (ret < 0) {

return ret;

}

diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c

index XXXXXXX..XXXXXXX 100644

--- a/subprojects/libvhost-user/libvhost-user.c

+++ b/subprojects/libvhost-user/libvhost-user.c

@@ -XXX,XX +XXX,XX @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {

int i;

bool found = false;

- if (vmsg->fd_num != 1) {

+ if (vmsg->fd_num > 1) {

vmsg_close_fds(vmsg);

- vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - only 1 fd "

+ vu_panic(dev, "VHOST_USER_REM_MEM_REG received %d fds - at most 1 fd "

"should be sent for this message type", vmsg->fd_num);

return false;

if (vmsg->size < VHOST_USER_MEM_REG_SIZE) {

- close(vmsg->fds[0]);

+ vmsg_close_fds(vmsg);

vu_panic(dev, "VHOST_USER_REM_MEM_REG requires a message size of at "

"least %d bytes and only %d bytes were received",

VHOST_USER_MEM_REG_SIZE, vmsg->size);

@@ -XXX,XX +XXX,XX @@ vu_rem_mem_reg(VuDev *dev, VhostUserMsg *vmsg) {

vu_panic(dev, "Specified region not found\n");

2.35.1

From: Hanna Reitz <hreitz@redhat.com>

This function is safe to call in an I/O context, and qcow2_do_open()

does so (invoked in an I/O context by qcow2_co_invalidate_cache()).

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20220427114057.36651-2-hreitz@redhat.com>

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

include/block/block-global-state.h | 1 -

include/block/block-io.h | 1 +

block.c | 2 +-

3 files changed, 2 insertions(+), 2 deletions(-)

@@ -XXX,XX +XXX,XX @@ const char *bdrv_get_device_or_node_name(const BlockDriverState *bs)

int bdrv_get_flags(BlockDriverState *bs)

{

- GLOBAL_STATE_CODE();

+ IO_CODE();

return bs->open_flags;

2.35.1

From: Hanna Reitz <hreitz@redhat.com>

qcow2_co_invalidate_cache() closes and opens the qcow2 file, by calling

qcow2_close() and qcow2_do_open(). These two functions must thus be

usable from both a global-state and an I/O context.

As they are, they are not safe to call in an I/O context, because they

use bdrv_unref_child() and bdrv_open_child() to close/open the data_file

child, respectively, both of which are global-state functions. When

used from qcow2_co_invalidate_cache(), we do not need to close/open the

data_file child, though (we do not do this for bs->file or bs->backing

either), and so we should skip it in the qcow2_co_invalidate_cache()

path.

To do so, add a parameter to qcow2_do_open() and qcow2_close() to make

them skip handling s->data_file, and have qcow2_co_invalidate_cache()

exempt it from the memset() on the BDRVQcow2State.

(Note that the QED driver similarly closes/opens the QED image by

invoking bdrv_qed_close()+bdrv_qed_do_open(), but both functions seem

safe to use in an I/O context.)

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/945

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20220427114057.36651-3-hreitz@redhat.com>

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

block/qcow2.c | 104 ++++++++++++++++++++++++++++++--------------------

1 file changed, 62 insertions(+), 42 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)

/* Called with s->lock held. */

static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,

- int flags, Error **errp)

+ int flags, bool open_data_file,

+ Error **errp)

{

ERRP_GUARD();

BDRVQcow2State *s = bs->opaque;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,

goto fail;

}

- /* Open external data file */

- s->data_file = bdrv_open_child(NULL, options, "data-file", bs,

- &child_of_bds, BDRV_CHILD_DATA,

- true, errp);

- if (*errp) {

- ret = -EINVAL;

- goto fail;

- }

+ if (open_data_file) {

+ /* Open external data file */

+ s->data_file = bdrv_open_child(NULL, options, "data-file", bs,

+ &child_of_bds, BDRV_CHILD_DATA,

+ true, errp);

+ if (*errp) {

+ ret = -EINVAL;

+ goto fail;

+ }

- if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {

- if (!s->data_file && s->image_data_file) {

- s->data_file = bdrv_open_child(s->image_data_file, options,

- "data-file", bs, &child_of_bds,

- BDRV_CHILD_DATA, false, errp);

+ if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {

+ if (!s->data_file && s->image_data_file) {

+ s->data_file = bdrv_open_child(s->image_data_file, options,

+ "data-file", bs, &child_of_bds,

+ BDRV_CHILD_DATA, false, errp);

+ if (!s->data_file) {

+ ret = -EINVAL;

+ goto fail;

+ }

+ }

if (!s->data_file) {

+ error_setg(errp, "'data-file' is required for this image");

ret = -EINVAL;

goto fail;

}

- }

- if (!s->data_file) {

- error_setg(errp, "'data-file' is required for this image");

- ret = -EINVAL;

- goto fail;

- }

- /* No data here */

- bs->file->role &= ~BDRV_CHILD_DATA;

+ /* No data here */

+ bs->file->role &= ~BDRV_CHILD_DATA;

- /* Must succeed because we have given up permissions if anything */

- bdrv_child_refresh_perms(bs, bs->file, &error_abort);

- } else {

- if (s->data_file) {

- error_setg(errp, "'data-file' can only be set for images with an "

- "external data file");

- ret = -EINVAL;

- goto fail;

- }

+ /* Must succeed because we have given up permissions if anything */

+ bdrv_child_refresh_perms(bs, bs->file, &error_abort);

+ } else {

+ if (s->data_file) {

+ error_setg(errp, "'data-file' can only be set for images with "

+ "an external data file");

+ ret = -EINVAL;

+ goto fail;

+ }

- s->data_file = bs->file;

+ s->data_file = bs->file;

- if (data_file_is_raw(bs)) {

- error_setg(errp, "data-file-raw requires a data file");

- ret = -EINVAL;

- goto fail;

+ if (data_file_is_raw(bs)) {

+ error_setg(errp, "data-file-raw requires a data file");

+ ret = -EINVAL;

+ goto fail;

+ }

}

}

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,

fail:

g_free(s->image_data_file);

- if (has_data_file(bs)) {

+ if (open_data_file && has_data_file(bs)) {

bdrv_unref_child(bs, s->data_file);

s->data_file = NULL;

}

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn qcow2_open_entry(void *opaque)

BDRVQcow2State *s = qoc->bs->opaque;

qemu_co_mutex_lock(&s->lock);

- qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp);

+ qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, true,

+ qoc->errp);

qemu_co_mutex_unlock(&s->lock);

@@ -XXX,XX +XXX,XX @@ static int qcow2_inactivate(BlockDriverState *bs)

return result;

}

-static void qcow2_close(BlockDriverState *bs)

+static void qcow2_do_close(BlockDriverState *bs, bool close_data_file)

{

BDRVQcow2State *s = bs->opaque;

qemu_vfree(s->l1_table);

@@ -XXX,XX +XXX,XX @@ static void qcow2_close(BlockDriverState *bs)

g_free(s->image_backing_file);

g_free(s->image_backing_format);

- if (has_data_file(bs)) {

+ if (close_data_file && has_data_file(bs)) {

bdrv_unref_child(bs, s->data_file);

s->data_file = NULL;

}

@@ -XXX,XX +XXX,XX @@ static void qcow2_close(BlockDriverState *bs)

qcow2_free_snapshots(bs);

}

+static void qcow2_close(BlockDriverState *bs)

+ qcow2_do_close(bs, true);

static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,

Error **errp)

ERRP_GUARD();

BDRVQcow2State *s = bs->opaque;

+ BdrvChild *data_file;

int flags = s->flags;

QCryptoBlock *crypto = NULL;

QDict *options;

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,

crypto = s->crypto;

s->crypto = NULL;

- qcow2_close(bs);

+ /*

+ * Do not reopen s->data_file (i.e., have qcow2_do_close() not close it,

+ * and then prevent qcow2_do_open() from opening it), because this function

+ * runs in the I/O path and as such we must not invoke global-state

+ * functions like bdrv_unref_child() and bdrv_open_child().

+ */

+ qcow2_do_close(bs, false);

+

+ data_file = s->data_file;

memset(s, 0, sizeof(BDRVQcow2State));

+ s->data_file = data_file;

+

options = qdict_clone_shallow(bs->options);

flags &= ~BDRV_O_INACTIVE;

qemu_co_mutex_lock(&s->lock);

- ret = qcow2_do_open(bs, options, flags, errp);

+ ret = qcow2_do_open(bs, options, flags, false, errp);

qemu_co_mutex_unlock(&s->lock);

qobject_unref(options);

if (ret < 0) {

2.35.1

From: Hanna Reitz <hreitz@redhat.com>

This reverts commit b1c073490553f80594b903ceedfc7c1aef6b1b19. (We

wanted to do so once the 7.1 tree opens, which has happened. The issue

reported in https://gitlab.com/qemu-project/qemu/-/issues/945 should be

fixed by the preceding patches.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20220427114057.36651-4-hreitz@redhat.com>

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

include/qemu/main-loop.h | 3 +--

1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h

--- a/include/qemu/main-loop.h

+++ b/include/qemu/main-loop.h

@@ -XXX,XX +XXX,XX @@ bool qemu_in_main_thread(void);

#else

#define GLOBAL_STATE_CODE() \

do { \

- /* FIXME: Re-enable after 7.0 release */ \

- /* assert(qemu_in_main_thread()); */ \

+ assert(qemu_in_main_thread()); \

} while (0)

#endif /* CONFIG_COCOA */

2.35.1

From: Hanna Reitz <hreitz@redhat.com>

Create a VM with a BDS in an iothread, add -incoming defer to the

command line, and then export this BDS via NBD. Doing so should not

fail an assertion.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20220427114057.36651-5-hreitz@redhat.com>

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

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

.../tests/export-incoming-iothread | 81 +++++++++++++++++++

.../tests/export-incoming-iothread.out | 5 ++

2 files changed, 86 insertions(+)

create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread

create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out

diff --git a/tests/qemu-iotests/tests/export-incoming-iothread b/tests/qemu-iotests/tests/export-incoming-iothread

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/tests/export-incoming-iothread

@@ -XXX,XX +XXX,XX @@

+#!/usr/bin/env python3

+# group: rw quick migration

+#

+# Regression test for issue 945:

+# https://gitlab.com/qemu-project/qemu/-/issues/945

+# Test adding an export on top of an iothread-ed block device while in

+# -incoming defer.

+#

+# 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 qemu_img_create

+

+

+image_size = 1 * 1024 * 1024

+test_img = os.path.join(iotests.test_dir, 'test.img')

+node_name = 'node0'

+iothread_id = 'iothr0'

+

+nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock')

+

+

+class TestExportIncomingIothread(iotests.QMPTestCase):

+ def setUp(self) -> None:

+ qemu_img_create('-f', iotests.imgfmt, test_img, str(image_size))

+

+ self.vm = iotests.VM()

+ self.vm.add_object(f'iothread,id={iothread_id}')

+ self.vm.add_blockdev((

+ f'driver={iotests.imgfmt}',

+ f'node-name={node_name}',

+ 'file.driver=file',

+ f'file.filename={test_img}'

+ ))

+ self.vm.add_incoming('defer')

+ self.vm.launch()

+

+ def tearDown(self):

+ self.vm.shutdown()

+ os.remove(test_img)

+

+ def test_export_add(self):

+ result = self.vm.qmp('nbd-server-start', {

+ 'addr': {

+ 'type': 'unix',

+ 'data': {

+ 'path': nbd_sock

+ }

+ }

+ })

+ self.assert_qmp(result, 'return', {})

+

+ # Regression test for issue 945: This should not fail an assertion

+ result = self.vm.qmp('block-export-add', {

+ 'type': 'nbd',

+ 'id': 'exp0',

+ 'node-name': node_name,

+ 'iothread': iothread_id

+ })

+ self.assert_qmp(result, 'return', {})

+

+

+if __name__ == '__main__':

+ iotests.main(supported_fmts=['generic'],

+ unsupported_fmts=['luks'], # Would need a secret

+ supported_protocols=['file'])

diff --git a/tests/qemu-iotests/tests/export-incoming-iothread.out b/tests/qemu-iotests/tests/export-incoming-iothread.out

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/tests/export-incoming-iothread.out

@@ -XXX,XX +XXX,XX @@

+.

+----------------------------------------------------------------------

+Ran 1 tests

+

+OK

2.35.1

From: Hanna Reitz <hreitz@redhat.com>

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>

Message-Id: <20220314162719.65384-2-hreitz@redhat.com>

block/vmdk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-

1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c

--- a/block/vmdk.c

+++ b/block/vmdk.c

@@ -XXX,XX +XXX,XX @@ 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;

@@ -XXX,XX +XXX,XX @@ 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;

@@ -XXX,XX +XXX,XX @@ 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

From: Hanna Reitz <hreitz@redhat.com>

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>

Message-Id: <20220314162719.65384-3-hreitz@redhat.com>

tests/qemu-iotests/tests/reopen-file | 89 ++++++++++++++++++++++++

tests/qemu-iotests/tests/reopen-file.out | 5 ++

2 files changed, 94 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 XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/tests/reopen-file

+#!/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 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:

+ res = qemu_img_create('-f', imgfmt, test_img, str(image_size))

+ assert res.returncode == 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',