Re: [PATCH v5 01/15] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers

[Kevin Wolf <kwolf@redhat.com>]

Am 23.11.2022 um 12:42 hat Emanuele Giuseppe Esposito geschrieben:
> bdrv_common_block_status_above() is a g_c_w, and it is being called by
> many "wrapper" functions like bdrv_is_allocated(),
> bdrv_is_allocated_above() and bdrv_block_status_above().
> 
> Because we want to eventually split the coroutine from non-coroutine
> case in g_c_w, create duplicate wrappers that take care of directly
> calling the same coroutine functions called in the g_c_w.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/io.c               | 64 ++++++++++++++++++++++++++++++++++++++--
>  include/block/block-io.h | 15 ++++++++++
>  2 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 38e57d1f67..1bc05c8282 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2533,6 +2533,19 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
>      return ret;
>  }
>  
> +int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
> +                                            BlockDriverState *base,
> +                                            int64_t offset, int64_t bytes,
> +                                            int64_t *pnum, int64_t *map,
> +                                            BlockDriverState **file)
> +{
> +    IO_CODE();
> +    /* If QEMU_IN_COROUTINE() fails, use bdrv_block_status_above() */
> +    QEMU_IN_COROUTINE();

This is an obvious patch order problem: The macro doesn't even exist
yet.

As I said, personally, I don't feel like putting QEMU_IN_COROUTINE()
assertions into every coroutine_fn is a useful thing to do. Static
analysis (maybe even with something vrc based in 'make check'? Paolo,
would this be realistic?) seems much preferable. But I'd like to hear
other opinions on this, too.

I feel the same way about the comment. Yes, of course, if you're not in
a coroutine, don't call the _co variant of a function, but the one
without it. But that goes without saying, doesn't it?

> +    return bdrv_co_common_block_status_above(bs, base, false, true, offset,
> +                                             bytes, pnum, map, file, NULL);
> +}

Apart from these considerations the patch looks right.

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