qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH for-7.1 2/8] nbd: mark more coroutine_fns


From: Eric Blake
Subject: Re: [PATCH for-7.1 2/8] nbd: mark more coroutine_fns
Date: Wed, 13 Apr 2022 07:25:13 -0500
User-agent: NeoMutt/20211029-6-a115bf

On Tue, Apr 12, 2022 at 09:41:58PM +0200, Paolo Bonzini wrote:
> Several coroutine functions in block/nbd.c are not marked as such.  This
> patch adds a few more markers; it is not exhaustive, but it focuses
> especially on:
> 
> - places that wake other coroutines, because aio_co_wake() has very
> different semantics inside a coroutine (queuing after yield vs. entering
> immediately);
> 
> - functions with _co_ in their names, to avoid confusion

Should we add _co_ in the names of the three other functions thus
marked?  As in:

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/nbd.c | 64 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)

> @@ -133,7 +133,7 @@ static bool nbd_client_connected(BDRVNBDState *s)
>      return qatomic_load_acquire(&s->state) == NBD_CLIENT_CONNECTED;
>  }
> 
> -static bool nbd_recv_coroutine_wake_one(NBDClientRequest *req)
> +static bool coroutine_fn nbd_recv_coroutine_wake_one(NBDClientRequest *req)

This already has _coroutine_ in the name, would it be better as _co_?

>  {
>      if (req->receiving) {
>          req->receiving = false;
> @@ -144,7 +144,7 @@ static bool nbd_recv_coroutine_wake_one(NBDClientRequest 
> *req)
>      return false;
>  }
> 
> -static void nbd_recv_coroutines_wake(BDRVNBDState *s, bool all)
> +static void coroutine_fn nbd_recv_coroutines_wake(BDRVNBDState *s, bool all)

This already has _coroutines_ in the name, would it be better as _co_?

>  {
>      int i;
> 
> @@ -155,7 +155,7 @@ static void nbd_recv_coroutines_wake(BDRVNBDState *s, 
> bool all)
>      }
>  }
> 
> -static void nbd_channel_error(BDRVNBDState *s, int ret)
> +static void coroutine_fn nbd_channel_error(BDRVNBDState *s, int ret)

This has no mention of coroutines, but does call
nbd_recv_coroutines_wake.  Should we add _co_ in the name?

But as written, your change makes sense to me for adding the label to
all functions in this patch.

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

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]