Re: [RFC PATCH 2/6] block: Mark bdrv_co_get_allocated_file_size() as mixed

[Kevin Wolf <kwolf@redhat.com>]

Am 23.05.2023 um 23:38 hat Fabiano Rosas geschrieben:
> Some callers of this function are about to be converted to use
> coroutines, so allow it to be executed both inside and outside a
> coroutine.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

This is not a sufficient justification for introducing a new mixed
function (we want to get rid of them, not add new ones).

You need to explain why the new coroutine callers can't directly call
bdrv_co_get_allocated_file_size() instead of going through the wrapper.
This is usually only the case if you have a function that doesn't know
whether it runs in coroutine context or not. Functions that you
explicitly convert to coroutine_fn know for sure.

>  include/block/block-io.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index a27e471a87..c1f96faca5 100644
> --- a/include/block/block-io.h
> +++ b/include/block/block-io.h
> @@ -87,7 +87,7 @@ int64_t co_wrapper_mixed_bdrv_rdlock bdrv_getlength(BlockDriverState *bs);
>  int64_t coroutine_fn GRAPH_RDLOCK
>  bdrv_co_get_allocated_file_size(BlockDriverState *bs);
>  
> -int64_t co_wrapper_bdrv_rdlock
> +int64_t co_wrapper_mixed_bdrv_rdlock
>  bdrv_get_allocated_file_size(BlockDriverState *bs);

You're changing bdrv_get_allocated_file_size() (which is the
function you really mean), but the subject line talks about
bdrv_co_get_allocated_file_size().

Kevin