[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
[PATCH for-7.1 6/8] nbd: move s->state under requests_lock, Paolo Bonzini, 2022/04/12