Re: [Qemu-devel] [PATCH 3/5] Convert BlockDriver to explicit coroutine annotations

[Gabriel Kerneis <gabriel@kerneis.info>]

Hi Charlie,

Many thanks for this patch series.

On Mon, Aug 05, 2013 at 08:44:05PM +0200, Charlie Shepherd wrote:
> This patch converts the .bdrv_open, .bdrv_file_open and .bdrv_create members of struct BlockDriver
> to be explicitly annotated as coroutine_fn, rather than yielding dynamically depending on whether
> they are executed in a coroutine context or not.

The commit message is incomplete. You also convert brdv_read and brdv_write.

Moreover:

> diff --git a/block/ssh.c b/block/ssh.c
> index d7e7bf8..66ac4c1 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -748,7 +748,7 @@ static int return_true(void *opaque)
>      return 1;
>  }
>  
> -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> +static void set_fd_handler(BDRVSSHState *s)
>  {
>      int r;
>      IOHandler *rd_handler = NULL, *wr_handler = NULL;
> @@ -769,7 +769,7 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>      qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, return_true, co);
>  }
>  
> -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
> +static void clear_fd_handler(BDRVSSHState *s)
>  {
>      DPRINTF("s->sock=%d", s->sock);
>      qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL, NULL);
> diff --git a/include/block/coroutine_int.h b/include/block/coroutine_int.h
> index f133d65..d0ab27d 100644
> --- a/include/block/coroutine_int.h
> +++ b/include/block/coroutine_int.h
> @@ -48,6 +48,6 @@ Coroutine *qemu_coroutine_new(void);
>  void qemu_coroutine_delete(Coroutine *co);
>  CoroutineAction qemu_coroutine_switch(Coroutine *from, Coroutine *to,
>                                        CoroutineAction action);
> -void coroutine_fn qemu_co_queue_run_restart(Coroutine *co);
> +void qemu_co_queue_run_restart(Coroutine *co);
>  
>  #endif

Adding coroutine_fn where it is necessary seems straightforward to me: just
follow the "callers of coroutine_fn should be coroutine_fn" rule (assuming you
got it right and did not over-annotate). On the other hand, you
should probably explain in the commit message why you *remove* those three
coroutine_fn annotations.

Best,
-- 
Gabriel