Re: [PATCH v3 1/3] block/io: refactor coroutine wrappers

[Eric Blake]

On 5/22/20 11:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Most of coroutine wrappers already follow this notation:

s/of/of our/
s/notation/convention/

> We have coroutine_fn bdrv_co_<something>(<normal argument list>), which
> is the core functions, and wrapper, which does polling loope is called
> bdrv_<something>(<same argument list>).

We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as 
the core function, and a wrapper 'bdrv_<something>(<same argument 
list>)' which does a polling loop.

> The only outsiders are bdrv_prwv_co and bdrv_common_block_status_above

s/are/are the/

> wrappers. Let's refactor the to behave as the others, it simplifies

s/the/them/

> further conversion of coroutine wrappers.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
>   1 file changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 121ce17a49..bd00a70b47 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -900,28 +900,32 @@ typedef struct RwCo {
>       BdrvRequestFlags flags;
>   } RwCo;
>   
> +static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
> +                                     QEMUIOVector *qiov, bool is_write,
> +                                     BdrvRequestFlags flags)
> +{
> +    if (is_write) {
> +        return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
> +    } else {
> +        return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
> +    }
> +}
> +

If we're trying to avoid needless indirection, wouldn't it be simpler to 
quit trying to slam reads and writes through a single prwv function that 
then has to split back out, and instead make two separate coroutine 
wrappers, one for just reads, and the other for just writes, without 
having to mess with a 'bool is_write' parameter?

>   static void coroutine_fn bdrv_rw_co_entry(void *opaque)
>   {

That is, should we have bdrv_co_preadv_entry and bdrv_co_pwritev_entry 
instead of just one bdrv_rw_co_entry?

At any rate, the renames done here are mechanical enough that if we make 
further changes, it could be a separate commit.

Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org