From: Alberto Faria <afaria@redhat.com>

Clang has a generic __annotate__ attribute that can be used by

static analyzers to understand properties of functions and

analyze the control flow. Furthermore, unlike TSA annotations, the

__annotate__ attribute applies to function pointers as well.

As a first step towards static analysis of coroutine_fn markers,

attach the attribute to the marker when compiling with clang.

Signed-off-by: Alberto Faria <afaria@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-Id: <20221216110758.559947-2-pbonzini@redhat.com>

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

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

---

include/qemu/coroutine.h | 4 ++++

1 file changed, 4 insertions(+)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/coroutine.h

+++ b/include/qemu/coroutine.h

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

* ....

* }

*/

+#ifdef __clang__

+#define coroutine_fn __attribute__((__annotate__("coroutine_fn")))

+#else

#define coroutine_fn

+#endif

typedef struct Coroutine Coroutine;

--

2.38.1

From: Alberto Faria <afaria@redhat.com>

Add more annotations to functions, describing valid and invalid

calls from coroutine to non-coroutine context.

When applied to a function, no_coroutine_fn advertises that it should

not be called from coroutine_fn functions. This can be because the

function blocks or, in the case of generated_co_wrapper, to enforce

that coroutine_fn functions directly call the coroutine_fn that backs

the generated_co_wrapper.

coroutine_mixed_fn instead is for function that can be called in

both coroutine and non-coroutine context, but will suspend when

called in coroutine context. Annotating them is a first step

towards enforcing that non-annotated functions are absolutely

not going to suspend.

These can be used for example with the vrc tool:

# find functions that *really* cannot be called from no_coroutine_fn

(vrc) load --loader clang libblock.fa.p/meson-generated_.._block_block-gen.c.o

(vrc) paths [no_coroutine_fn,!coroutine_mixed_fn]

bdrv_remove_persistent_dirty_bitmap

bdrv_create

bdrv_can_store_new_dirty_bitmap

# find how coroutine_fns end up calling a mixed function

(vrc) load --loader clang --force libblock.fa.p/*.c.o

(vrc) paths [coroutine_fn] [!no_coroutine_fn]* [coroutine_mixed_fn]

...

bdrv_pread <- vhdx_log_write <- vhdx_log_write_and_flush <- vhdx_co_writev

...

Signed-off-by: Alberto Faria <afaria@redhat.com>

[Rebase, add coroutine_mixed_fn. - Paolo]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-Id: <20221216110758.559947-3-pbonzini@redhat.com>

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

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

---

include/block/block-common.h | 11 ++++++----

include/qemu/coroutine.h | 39 ++++++++++++++++++++++++++++++++++++

2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/include/block/block-common.h b/include/block/block-common.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block-common.h

+++ b/include/block/block-common.h

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

* - co_wrapper_mixed_bdrv_rdlock are co_wrapper_mixed functions but

* automatically take and release the graph rdlock when creating a new

* coroutine.

+ *

+ * These functions should not be called from a coroutine_fn; instead,

+ * call the wrapped function directly.

*/

-#define co_wrapper

-#define co_wrapper_mixed

-#define co_wrapper_bdrv_rdlock

-#define co_wrapper_mixed_bdrv_rdlock

+#define co_wrapper no_coroutine_fn

+#define co_wrapper_mixed no_coroutine_fn coroutine_mixed_fn

+#define co_wrapper_bdrv_rdlock no_coroutine_fn

+#define co_wrapper_mixed_bdrv_rdlock no_coroutine_fn coroutine_mixed_fn

#include "block/dirty-bitmap.h"

#include "block/blockjob.h"

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h

index XXXXXXX..XXXXXXX 100644

--- a/include/qemu/coroutine.h

+++ b/include/qemu/coroutine.h

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

#define coroutine_fn

#endif

+/**

+ * Mark a function that can suspend when executed in coroutine context,

+ * but can handle running in non-coroutine context too.

+ */

+#ifdef __clang__

+#define coroutine_mixed_fn __attribute__((__annotate__("coroutine_mixed_fn")))

+#else

+#define coroutine_mixed_fn

+#endif

+

+/**

+ * Mark a function that should not be called from a coroutine context.

+ * Usually there will be an analogous, coroutine_fn function that should

+ * be used instead.

+ *

+ * When the function is also marked as coroutine_mixed_fn, the function should

+ * only be called if the caller does not know whether it is in coroutine

+ * context.

+ *

+ * Functions that are only no_coroutine_fn, on the other hand, should not

+ * be called from within coroutines at all. This for example includes

+ * functions that block.

+ *

+ * In the future it would be nice to enable compiler or static checker

+ * support for catching such errors. This annotation is the first step

+ * towards this, and in the meantime it serves as documentation.

+ *

+ * For example:

+ *

+ * static void no_coroutine_fn foo(void) {

+ * ....

+ * }

+ */

+#ifdef __clang__

+#define no_coroutine_fn __attribute__((__annotate__("no_coroutine_fn")))

+#else

+#define no_coroutine_fn

+#endif

+

typedef struct Coroutine Coroutine;

/**

--

2.38.1

From: Paolo Bonzini <pbonzini@redhat.com>

qemu-io's do_co_pwrite_zeroes is reinventing the coroutine wrapper

blk_pwrite_zeroes. Just use the real thing directly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-Id: <20221215130225.476477-1-pbonzini@redhat.com>

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

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

---

qemu-io-cmds.c | 57 +++++++++-----------------------------------------

1 file changed, 10 insertions(+), 47 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c

index XXXXXXX..XXXXXXX 100644

--- a/qemu-io-cmds.c

+++ b/qemu-io-cmds.c

@@ -XXX,XX +XXX,XX @@ static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset,

return 1;

}

-typedef struct {

- BlockBackend *blk;

- int64_t offset;

- int64_t bytes;

- int64_t *total;

- int flags;

- int ret;

- bool done;

-} CoWriteZeroes;

-

-static void coroutine_fn co_pwrite_zeroes_entry(void *opaque)

-{

- CoWriteZeroes *data = opaque;

-

- data->ret = blk_co_pwrite_zeroes(data->blk, data->offset, data->bytes,

- data->flags);

- data->done = true;

- if (data->ret < 0) {

- *data->total = data->ret;

- return;

- }

-

- *data->total = data->bytes;

-}

-

-static int do_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,

+static int do_pwrite_zeroes(BlockBackend *blk, int64_t offset,

int64_t bytes, int flags, int64_t *total)

{

- Coroutine *co;

- CoWriteZeroes data = {

- .blk = blk,

- .offset = offset,

- .bytes = bytes,

- .total = total,

- .flags = flags,

- .done = false,

- };

-

- co = qemu_coroutine_create(co_pwrite_zeroes_entry, &data);

- bdrv_coroutine_enter(blk_bs(blk), co);

- while (!data.done) {

- aio_poll(blk_get_aio_context(blk), true);

- }

- if (data.ret < 0) {

- return data.ret;

- } else {

- return 1;

+ int ret = blk_pwrite_zeroes(blk, offset, bytes,

+ flags | BDRV_REQ_ZERO_WRITE);

+

+ if (ret < 0) {

+ return ret;

}

+ *total = bytes;

+ return 1;

}

static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,

@@ -XXX,XX +XXX,XX @@ static void write_help(void)

" -C, -- report statistics in a machine parsable format\n"

" -q, -- quiet mode, do not show I/O statistics\n"

" -u, -- with -z, allow unmapping\n"

-" -z, -- write zeroes using blk_co_pwrite_zeroes\n"

+" -z, -- write zeroes using blk_pwrite_zeroes\n"

"\n");

}

@@ -XXX,XX +XXX,XX @@ static int write_f(BlockBackend *blk, int argc, char **argv)

if (bflag) {

ret = do_save_vmstate(blk, buf, offset, count, &total);

} else if (zflag) {

- ret = do_co_pwrite_zeroes(blk, offset, count, flags, &total);

+ ret = do_pwrite_zeroes(blk, offset, count, flags, &total);

} else if (cflag) {

ret = do_write_compressed(blk, buf, offset, count, &total);

} else {

--

2.38.1

From: Paolo Bonzini <pbonzini@redhat.com>

It has only one caller---inline it and remove the function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Message-Id: <20221215130225.476477-2-pbonzini@redhat.com>

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

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

---

include/block/block-io.h | 5 -----

block.c | 6 ------

block/block-backend.c | 2 +-

3 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/include/block/block-io.h b/include/block/block-io.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block-io.h

+++ b/include/block/block-io.h

@@ -XXX,XX +XXX,XX @@ AioContext *coroutine_fn bdrv_co_enter(BlockDriverState *bs);

*/

void coroutine_fn bdrv_co_leave(BlockDriverState *bs, AioContext *old_ctx);

-/**

- * Transfer control to @co in the aio context of @bs

- */

-void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co);

-

AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c);

void bdrv_io_plug(BlockDriverState *bs);

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ void coroutine_fn bdrv_co_unlock(BlockDriverState *bs)

}

}

-void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)

-{

- IO_CODE();

- aio_co_enter(bdrv_get_aio_context(bs), co);

-}

-

static void bdrv_do_remove_aio_context_notifier(BdrvAioNotifier *ban)

{

GLOBAL_STATE_CODE();

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

index XXXXXXX..XXXXXXX 100644

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset,

acb->has_returned = false;

co = qemu_coroutine_create(co_entry, acb);

- bdrv_coroutine_enter(blk_bs(blk), co);

+ aio_co_enter(blk_get_aio_context(blk), co);

acb->has_returned = true;

if (acb->rwco.ret != NOT_DONE) {

--

2.38.1

blk_unref() can't report any errors that happen while closing the image.

For example, if qcow2 hits an -ENOSPC error while writing out dirty

bitmaps when it's closed, it prints error messages to stderr, but

'qemu-img commit' won't see any error return value and will therefore

look successful with exit code 0.

In order to fix this, manually inactivate the image first before calling

blk_unref(). This already performs the operations that would be most

likely to fail while closing the image, but it can still return errors.

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

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

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

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

---

qemu-img.c | 13 +++++++++++++

1 file changed, 13 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c

index XXXXXXX..XXXXXXX 100644

--- a/qemu-img.c

+++ b/qemu-img.c

@@ -XXX,XX +XXX,XX @@ static BlockBackend *img_open(bool image_opts,

blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet,

force_share);

}

+

+ if (blk) {

+ blk_set_force_allow_inactivate(blk);

+ }

+

return blk;

}

@@ -XXX,XX +XXX,XX @@ unref_backing:

done:

qemu_progress_end();

+ /*

+ * Manually inactivate the image first because this way we can know whether

+ * an error occurred. blk_unref() doesn't tell us about failures.

+ */

+ ret = bdrv_inactivate_all();

+ if (ret < 0 && !local_err) {

+ error_setg_errno(&local_err, -ret, "Error while closing the image");

+ }

blk_unref(blk);

if (local_err) {

--

2.38.1

This tests that when an error happens while writing back bitmaps to the

image file in qcow2_inactivate(), 'qemu-img bitmap/commit' actually

return an error value in their exit code instead of making the operation

look successful to scripts.

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

Message-Id: <20230112191454.169353-5-kwolf@redhat.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

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

---

.../qemu-iotests/tests/qemu-img-close-errors | 95 +++++++++++++++++++

.../tests/qemu-img-close-errors.out | 23 +++++

2 files changed, 118 insertions(+)

create mode 100755 tests/qemu-iotests/tests/qemu-img-close-errors

create mode 100644 tests/qemu-iotests/tests/qemu-img-close-errors.out

diff --git a/tests/qemu-iotests/tests/qemu-img-close-errors b/tests/qemu-iotests/tests/qemu-img-close-errors

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/tests/qemu-img-close-errors

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

+#!/usr/bin/env bash

+# group: rw auto quick

+#

+# Check that errors while closing the image, in particular writing back dirty

+# bitmaps, is correctly reported with a failing qemu-img exit code.

+#

+# Copyright (C) 2023 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/>.

+#

+

+# creator

+owner=kwolf@redhat.com

+

+seq="$(basename $0)"

+echo "QA output created by $seq"

+

+status=1    # failure is the default!

+

+_cleanup()

+{

+ _cleanup_test_img

+}

+trap "_cleanup; exit \$status" 0 1 2 3 15

+

+# get standard environment, filters and checks

+cd ..

+. ./common.rc

+. ./common.filter

+

+_supported_fmt qcow2

+_supported_proto generic

+

+size=1G

+

+# The error we are going to use is ENOSPC. Depending on how many bitmaps we

+# create in the backing file (and therefore increase the used up space), we get

+# failures in different places. With a low number, only merging the bitmap

+# fails, whereas with a higher number, already 'qemu-img commit' fails.

+for max_bitmap in 6 7; do

+ echo

+ echo "=== Test with $max_bitmap bitmaps ==="

+

+ TEST_IMG="$TEST_IMG.base" _make_test_img -q $size

+ for i in $(seq 1 $max_bitmap); do

+ $QEMU_IMG bitmap --add "$TEST_IMG.base" "stale-bitmap-$i"

+ done

+

+ # Simulate a block device of 128 MB by resizing the image file accordingly

+ # and then enforcing the size with the raw driver

+ truncate "$TEST_IMG.base" --size 128M

+ BASE_JSON='json:{

+ "driver": "qcow2",

+ "file": {

+ "driver": "raw",

+ "size": 134217728,

+ "file": {

+ "driver": "file",

+ "filename":"'"$TEST_IMG.base"'"

+ }

+ }

+ }'

+

+ _make_test_img -q -b "$BASE_JSON" -F $IMGFMT

+ $QEMU_IMG bitmap --add "$TEST_IMG" "good-bitmap"

+

+ $QEMU_IO -c 'write 0 126m' "$TEST_IMG" | _filter_qemu_io

+

+ $QEMU_IMG commit -d "$TEST_IMG" 2>&1 | _filter_generated_node_ids

+ echo "qemu-img commit exit code: ${PIPESTATUS[0]}"

+

+ $QEMU_IMG bitmap --add "$BASE_JSON" "good-bitmap"

+ echo "qemu-img bitmap --add exit code: $?"

+

+ $QEMU_IMG bitmap --merge "good-bitmap" -b "$TEST_IMG" "$BASE_JSON" \

+ "good-bitmap" 2>&1 | _filter_generated_node_ids

+ echo "qemu-img bitmap --merge exit code: ${PIPESTATUS[0]}"

+done

+

+# success, all done

+echo "*** done"

+rm -f $seq.full

+status=0

+

diff --git a/tests/qemu-iotests/tests/qemu-img-close-errors.out b/tests/qemu-iotests/tests/qemu-img-close-errors.out

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/tests/qemu-img-close-errors.out

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

+QA output created by qemu-img-close-errors

+

+=== Test with 6 bitmaps ===

+wrote 132120576/132120576 bytes at offset 0

+126 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+Image committed.

+qemu-img commit exit code: 0

+qemu-img bitmap --add exit code: 0

+qemu-img: Lost persistent bitmaps during inactivation of node 'NODE_NAME': Failed to write bitmap 'good-bitmap' to file: No space left on device

+qemu-img: Error while closing the image: Invalid argument

+qemu-img: Lost persistent bitmaps during inactivation of node 'NODE_NAME': Failed to write bitmap 'good-bitmap' to file: No space left on device

+qemu-img bitmap --merge exit code: 1

+

+=== Test with 7 bitmaps ===

+wrote 132120576/132120576 bytes at offset 0

+126 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+qemu-img: Lost persistent bitmaps during inactivation of node 'NODE_NAME': Failed to write bitmap 'stale-bitmap-7' to file: No space left on device

+qemu-img: Lost persistent bitmaps during inactivation of node 'NODE_NAME': Failed to write bitmap 'stale-bitmap-7' to file: No space left on device

+qemu-img: Error while closing the image: Invalid argument

+qemu-img commit exit code: 1

+qemu-img bitmap --add exit code: 0

+qemu-img bitmap --merge exit code: 0

+*** done

--

2.38.1

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Just omit the various 'return' when the return type is void.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

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

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

---

scripts/block-coroutine-wrapper.py | 20 +++++++++++++++-----

1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py

index XXXXXXX..XXXXXXX 100644

--- a/scripts/block-coroutine-wrapper.py

+++ b/scripts/block-coroutine-wrapper.py

@@ -XXX,XX +XXX,XX @@ def __init__(self, return_type: str, name: str, args: str,

ctx = 'qemu_get_aio_context()'

self.ctx = ctx

+ self.get_result = 's->ret = '

+ self.ret = 'return s.ret;'

+ self.co_ret = 'return '

+ self.return_field = self.return_type + " ret;"

+ if self.return_type == 'void':

+ self.get_result = ''

+ self.ret = ''

+ self.co_ret = ''

+ self.return_field = ''

+

def gen_list(self, format: str) -> str:

return ', '.join(format.format_map(arg.__dict__) for arg in self.args)

@@ -XXX,XX +XXX,XX @@ def create_mixed_wrapper(func: FuncDecl) -> str:

{{

if (qemu_in_coroutine()) {{

{graph_assume_lock}

- return {name}({ func.gen_list('{name}') });

+ {func.co_ret}{name}({ func.gen_list('{name}') });

}} else {{

{struct_name} s = {{

.poll_state.ctx = {func.ctx},

@@ -XXX,XX +XXX,XX @@ def create_mixed_wrapper(func: FuncDecl) -> str:

s.poll_state.co = qemu_coroutine_create({name}_entry, &s);

bdrv_poll_co(&s.poll_state);

- return s.ret;

+ {func.ret}

}}

}}"""

@@ -XXX,XX +XXX,XX @@ def create_co_wrapper(func: FuncDecl) -> str:

s.poll_state.co = qemu_coroutine_create({name}_entry, &s);

bdrv_poll_co(&s.poll_state);

- return s.ret;

+ {func.ret}

}}"""

@@ -XXX,XX +XXX,XX @@ def gen_wrapper(func: FuncDecl) -> str:

typedef struct {struct_name} {{

BdrvPollCo poll_state;

- {func.return_type} ret;

+ {func.return_field}

{ func.gen_block(' {decl};') }

}} {struct_name};

@@ -XXX,XX +XXX,XX @@ def gen_wrapper(func: FuncDecl) -> str:

{struct_name} *s = opaque;

{graph_lock}

- s->ret = {name}({ func.gen_list('s->{name}') });

+ {func.get_result}{name}({ func.gen_list('s->{name}') });

{graph_unlock}

s->poll_state.in_progress = false;

--

2.38.1