Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Vladimir Sementsov-Ogievskiy]

On 09.04.24 18:49, Eric Blake wrote:
> On Tue, Apr 09, 2024 at 09:30:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 08.04.24 19:00, Eric Blake wrote:
> > > nbd_negotiate() is already marked coroutine_fn.  And given the fix in
> > > the previous patch to have nbd_negotiate_handle_starttls not create
> > > and wait on a g_main_loop (as that would violate coroutine
> > > constraints), it is worth marking the rest of the related static
> > > functions reachable only during option negotiation as also being
> > > coroutine_fn.
> > >
> > > Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > ---
> > >    nbd/server.c | 102 +++++++++++++++++++++++++++++----------------------
> > >    1 file changed, 59 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/nbd/server.c b/nbd/server.c
> > > index 98ae0e16326..1857fba51c1 100644
> >
> > [..]
> >
> > >    {
> > >        int rc;
> > >        g_autofree char *name = NULL;
> > > @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
> > >        Coroutine *co;
> > >    };
> > >
> > > -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > > +static coroutine_fn void
> >
> > This is not, that's a callback for tls handshake, which is not coroutine 
> > context as I understand.
>
> The call chain is:
>
> nbd_negotiate() - coroutine_fn before this patch
>    nbd_negotiate_options() - marked coroutine_fn here
>      nbd_negotiate_handle_starttls() - marked coroutine_fn here
>        qio_channel_tls_handshake() - in io/channel-tls.c; not marked 
> coroutine_fn, but
>                                      works both in or out of coroutine context
>          ...
>          nbd_server_tls_handshake() - renamed in 1/2 of this series
>
> > without this hunk:
>
> I can drop that particular marking.  There are cases where the
> callback is reached without having done the qemu_coroutine_yield() in
> nbd_negotiate_handle_starttls; but there are also cases where the IO
> took enough time that the coroutine yielded and it is that callback
> that reawakens it.

The key thing for me is that in this case (when coroutine yielded), 
nbd_server_tls_handshake() would finally be called from glib IO handler, set in

   qio_channel_tls_handshake()
      qio_channel_tls_handshake_task()
         qio_channel_add_watch_full()
            g_source_set_callback(source, (GSourceFunc)func, user_data, notify);   
<<<

, which would definitely not be a coroutine context.


Do I understand correctly, that "coroutine_fn" means "only call from coroutine 
context"[1], or does it mean "could be called from coroutine context"[2]?

The comment in osdep.h says:

 * Mark a function that executes in coroutine context                           
                          |}
 *                                                                              
                          |
 * Functions that execute in coroutine context cannot be called directly from   
                          |
 * normal functions. ...

So I assume, we mean [1].

> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Thanks.

--
Best regards,
Vladimir