Re: [PATCH v2 05/26] blkdebug: add missing coroutine_fn annotations

[Eric Blake]

On Mon, May 09, 2022 at 12:29:58PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Sparse commit message; from the followup email, this sentence would be helpful:

| The only rule is that callers of coroutine_fn must be coroutine_fn themselves,
| or the call must be within "if (qemu_in_coroutine())".

> ---
>  block/blkdebug.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index bbf2948703..a93ba61487 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -587,8 +587,8 @@ out:
>      return ret;
>  }
>  
> -static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> -                      BlkdebugIOType iotype)
> +static int coroutine_fn rule_check(BlockDriverState *bs, uint64_t offset, 
> uint64_t bytes,
> +                                   BlkdebugIOType iotype)

Following that guideline, rule_check() is reached from:
+ coroutine_fn blkdebug_co_preadv
+ coroutine_fn blkdebug_co_pwritev
+ blkdebug_co_flush (fixed to be coroutine_fn below)
+ coroutine_fn blkdebug_co_pwrite_zeroes
+ coroutine_fn blkdebug_co_pdiscard
+ coroutine_fn blkdebug_co_block_status

>  {
>      BDRVBlkdebugState *s = bs->opaque;
>      BlkdebugRule *rule = NULL;
> @@ -672,7 +672,7 @@ blkdebug_co_pwritev(BlockDriverState *bs, int64_t offset, 
> int64_t bytes,
>      return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
>  }
>  
> -static int blkdebug_co_flush(BlockDriverState *bs)
> +static int coroutine_fn blkdebug_co_flush(BlockDriverState *bs)

blkdebug_co_flush() is only reached from:
+ .bdrv_co_flush_to_disk
  + from io.c coroutine_fn bdrv_co_flush

>  {
>      int err = rule_check(bs, 0, 0, BLKDEBUG_IO_TYPE_FLUSH);
>  
> @@ -791,7 +791,7 @@ static void blkdebug_close(BlockDriverState *bs)
>  }
>  
>  /* Called with lock held.  */
> -static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule)
> +static void coroutine_fn suspend_request(BlockDriverState *bs, BlkdebugRule 
> *rule)

suspend_request() is only reached from:
+ process_rule (fixed to be coroutine_fn below)

>  {
>      BDRVBlkdebugState *s = bs->opaque;
>      BlkdebugSuspendedReq *r;
> @@ -810,8 +810,8 @@ static void suspend_request(BlockDriverState *bs, 
> BlkdebugRule *rule)
>  }
>  
>  /* Called with lock held.  */
> -static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule,
> -                         int *action_count, int *new_state)
> +static void coroutine_fn process_rule(BlockDriverState *bs, struct 
> BlkdebugRule *rule,
> +                                      int *action_count, int *new_state)

process_rule() is only reached from:
+ blkdebug_debug_event (fixed to be coroutine_fn below)

>  {
>      BDRVBlkdebugState *s = bs->opaque;
>  
> @@ -840,7 +840,7 @@ static void process_rule(BlockDriverState *bs, struct 
> BlkdebugRule *rule,
>      }
>  }
>  
> -static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event)
> +static void coroutine_fn blkdebug_debug_event(BlockDriverState *bs, 
> BlkdebugEvent event)

Long line; could wrap the arguments.

blkdebug_debug_event() is only reached from:
+ .bdrv_debug_event
  + from io.c coroutine_fn bdrv_do_co_copy_on_readv
  + from io.c bdrv_padding_rmw_read (NOT marked coroutine_fn, no 
qemu_in_coroutine guard!)
  | + from io.c coroutine_fn bdrv_co_do_zero_pwritev
  | + from io.c coroutine_fn bdrv_co_pwritev_part
  + from io.c coroutine_fn bdrv_aligned_pwritev

So the patch is 95% correct; if you also mark io.c
bdrv_padding_rmw_read(), you can add:

Reviewed-by: Eric Blake <eblake@redhat.com>

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