Re: [PATCH v4] nbd/server: do not poll within a coroutine context

[Eric Blake]

On Mon, Apr 08, 2024 at 11:46:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 05.04.24 20:44, Eric Blake wrote:
> > From: Zhu Yangyang <zhuyangyang14@huawei.com>
> > 
> > Coroutines are not supposed to block. Instead, they should yield.
> > 
> > The client performs TLS upgrade outside of an AIOContext, during
> > synchronous handshake; this still requires g_main_loop.  But the
> > server responds to TLS upgrade inside a coroutine, so a nested
> > g_main_loop is wrong.  Since the two callbacks no longer share more
> > than the setting of data.complete and data.error, it's just as easy to
> > use static helpers instead of trying to share a common code path.
> > 
> > Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
> > Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com>
> > [eblake: move callbacks to their use point]
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I'm debating whether it is worth trying to shove this into 9.0; -rc3
is very late, and the problem is pre-existing, so I'm leaning towards
no.  At which point, it's better to get this right.

> 
> still, some notes below
> 
> > ---
> > 
> > v3: https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00375.html
> > 
> > in v4, factor even the struct to the .c files, avoiding a union [Vladimir]
> > 
> >   nbd/nbd-internal.h | 10 ----------
> >   nbd/client.c       | 27 +++++++++++++++++++++++----
> >   nbd/common.c       | 11 -----------
> >   nbd/server.c       | 29 +++++++++++++++++++++++------
> >   4 files changed, 46 insertions(+), 31 deletions(-)
> > 

> > +++ b/nbd/client.c
> > @@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, 
> > int opt, bool strict,
> >       return 1;
> >   }
> > 
> > +/* Callback to learn when QIO TLS upgrade is complete */
> > +struct NBDTLSClientHandshakeData {
> > +    bool complete;
> > +    Error *error;
> > +    GMainLoop *loop;
> > +};
> > +
> > +static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
> > +{
> > +    struct NBDTLSClientHandshakeData *data = opaque;
> > +
> > +    qio_task_propagate_error(task, &data->error);
> > +    data->complete = true;
> > +    if (data->loop) {
> > +        g_main_loop_quit(data->loop);
> > +    }
> > +}
> > +
> >   static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> >                                           QCryptoTLSCreds *tlscreds,
> >                                           const char *hostname, Error 
> > **errp)
> >   {
> >       int ret;
> >       QIOChannelTLS *tioc;
> > -    struct NBDTLSHandshakeData data = { 0 };
> > +    struct NBDTLSClientHandshakeData data = { 0 };
> > 
> >       ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
> >       if (ret <= 0) {
> > @@ -619,18 +637,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel 
> > *ioc,
> >           return NULL;
> >       }
> >       qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
> > -    data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> >       trace_nbd_receive_starttls_tls_handshake();
> >       qio_channel_tls_handshake(tioc,
> > -                              nbd_tls_handshake,
> > +                              nbd_client_tls_handshake,
> >                                 &data,
> >                                 NULL,
> >                                 NULL);
> > 
> >       if (!data.complete) {
> > +        data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> >           g_main_loop_run(data.loop);
> > +        g_main_loop_unref(data.loop);
> 
> probably good to assert(data.complete);

Seems reasonable.

> > +++ b/nbd/server.c
> > @@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient 
> > *client, Error **errp)
> >       return rc;
> >   }
> > 
> > +/* Callback to learn when QIO TLS upgrade is complete */
> > +struct NBDTLSServerHandshakeData {
> > +    bool complete;
> > +    Error *error;
> > +    Coroutine *co;
> > +};
> > +
> > +static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > +{
> > +    struct NBDTLSServerHandshakeData *data = opaque;
> > +
> > +    qio_task_propagate_error(task, &data->error);
> > +    data->complete = true;
> > +    if (!qemu_coroutine_entered(data->co)) {
> > +        aio_co_wake(data->co);
> > +    }
> > +}
> > 
> >   /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
> >    * new channel for all further (now-encrypted) communication. */
> > @@ -756,7 +773,7 @@ static QIOChannel 
> > *nbd_negotiate_handle_starttls(NBDClient *client,
> >   {
> >       QIOChannel *ioc;
> >       QIOChannelTLS *tioc;
> > -    struct NBDTLSHandshakeData data = { 0 };
> > +    struct NBDTLSServerHandshakeData data = { 0 };
> > 
> >       assert(client->opt == NBD_OPT_STARTTLS);
> > 
> > @@ -777,17 +794,17 @@ static QIOChannel 
> > *nbd_negotiate_handle_starttls(NBDClient *client,
> 
> preexisting: lack coroutine_fn, as well as caller nbd_negotiate_options()

Indeed, so now would not hurt to add them now that a callback is no
longer shared between callers in different context.  But probably as a
separate patch.

> 
> > 
> >       qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls");
> >       trace_nbd_negotiate_handle_starttls_handshake();
> > -    data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> > +    data.co = qemu_coroutine_self();
> >       qio_channel_tls_handshake(tioc,
> > -                              nbd_tls_handshake,
> > +                              nbd_server_tls_handshake,
> >                                 &data,
> >                                 NULL,
> >                                 NULL);
> > 
> > -    if (!data.complete) {
> > -        g_main_loop_run(data.loop);
> > +    while (!data.complete) {
> 
> not "if", but "while".. Do we expect entering from somewhere except 
> nbd_server_tls_handshake?
> 
> if not, probably safer construction would be
> 
> if (!data.complete) {
>    qemu_coroutine_yield();
>    assert(data.complete);
> }
> 
> - to avoid hiding unexpected coroutine switching bugs

TLS handshake is early enough in the NBD connection that there should
not be any parallel I/O yet (we can't switch to parallel outstanding
transactions until after successful NBD_OPT_GO), so I like your idea
of asserting that the coroutine is entered at most once.

v5 coming up

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org