Re: [PATCH v5 3/5] block/nbd: refactor nbd_recv_coroutines_wake_all()

[Vladimir Sementsov-Ogievskiy]

17.07.2021 00:25, Eric Blake wrote:
> On Wed, Jul 14, 2021 at 07:59:14PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Split out nbd_recv_coroutine_wake(), as it will be used in separate.
>
> s/in separate/separately/
>
> > Also add a possibility to wake only first found sleeping coroutine.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/nbd.c | 24 ++++++++++++++++--------
> >   1 file changed, 16 insertions(+), 8 deletions(-)
>
> > +
> > +static void nbd_recv_coroutines_wake_all(BDRVNBDState *s, bool only_one)
>
> Without reading docs (including the parameter name), I would have guessed:
>
> wake_all(s, true) - wakes all
> wake_all(s, false) - wakes one
>
> but your code does:
>
> wake_all(s, true) - wakes one
> wake_all(s, false) - wakes all
>
> Maybe that means this function and/or its parameter is now misnamed.
> Having the _all in the name with true to NOT be all is what threw me.
> Would the following be any better:
>
> nbd_recv_coroutines_wake(BDRVNBDState *s, bool all)
>
> where
>
> wake(s, true) - wakes all
> wake(s, false) - wakes one
>
> and where your helper function needs to be renamed, and callers
> updated to match those semantics?

No objections, will do)

Actually in a previous commit message I promise to rename the function, but 
actually don't do it.

> >   {
> >       int i;
> >   
> >       for (i = 0; i < MAX_NBD_REQUESTS; i++) {
> > -        NBDClientRequest *req = &s->requests[i];
> > -
> > -        if (req->coroutine && req->receiving) {
> > -            req->receiving = false;
> > -            aio_co_wake(req->coroutine);
> > +        if (nbd_recv_coroutine_wake(&s->requests[i]) && only_one) {
> > +            return;
>
> But while I'm not sold on the naming, the change in logic (to be able
> to wake any one waiter but not the whole list) looks useful for future
> patches.


--
Best regards,
Vladimir