[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 01/15] block-io: introduce coroutine_fn duplicates for bdr
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v5 01/15] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers |
Date: |
Wed, 23 Nov 2022 17:29:33 +0100 |
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>
- [PATCH v5 09/15] block: bdrv_create_file is a coroutine_fn, (continued)
- [PATCH v5 09/15] block: bdrv_create_file is a coroutine_fn, Emanuele Giuseppe Esposito, 2022/11/23
- [PATCH v5 11/15] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter, Emanuele Giuseppe Esposito, 2022/11/23
- [PATCH v5 15/15] block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple, Emanuele Giuseppe Esposito, 2022/11/23
- [PATCH v5 14/15] block: convert bdrv_create to generated_co_wrapper_simple, Emanuele Giuseppe Esposito, 2022/11/23
- [PATCH v5 12/15] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter, Emanuele Giuseppe Esposito, 2022/11/23
- [PATCH v5 13/15] block-coroutine-wrapper.py: support also basic return types, Emanuele Giuseppe Esposito, 2022/11/23
- [PATCH v5 01/15] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers, Emanuele Giuseppe Esposito, 2022/11/23
- Re: [PATCH v5 01/15] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers,
Kevin Wolf <=