Re: [Qemu-devel] [PATCH 3/4] nbd: do qemu_coroutine_yield during tls handshake

[Eric Blake]

On 2/11/19 6:56 AM, Vladimir Sementsov-Ogievskiy wrote:
> We always call qio_channel_tls_handshake in nbd from couroutine. Take
> benefit of it and just yield instead of creating personal main loop.
> 
> Mark and rename the function and it's callers correspondingly and
> trace-points too.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  nbd/client.c     | 26 +++++++++-----------------
>  nbd/common.c     |  6 ++----
>  nbd/server.c     | 45 +++++++++++++++++----------------------------
>  nbd/trace-events | 15 +++++++--------
>  4 files changed, 35 insertions(+), 57 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 2ba2220a4a..e3919be30e 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -578,13 +578,14 @@ static int nbd_request_simple_option(QIOChannel *ioc, 
> int opt, Error **errp)
>      return 1;
>  }
>  
> -static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> -                                        QCryptoTLSCreds *tlscreds,
> -                                        const char *hostname, Error **errp)
> +static QIOChannel *nbd_co_receive_starttls(

Missing coroutine_fn ?

> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        Error **errp)
>  {
>      int ret;
>      QIOChannelTLS *tioc;
> -    struct NBDTLSHandshakeData data = { 0 };
> +
> +    assert(qemu_in_coroutine());

Again, I'm not sure these assertions add much.

>  
>      ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);

Should we also be marking these helper functions as coroutine_fn by the
end of the series, once all callers are marked that way?

>      if (ret <= 0) {
> @@ -601,23 +602,13 @@ 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,
> -                              &data,
> +                              qemu_coroutine_self(),
>                                NULL,
>                                NULL);
> -
> -    if (!data.complete) {
> -        g_main_loop_run(data.loop);
> -    }
> -    g_main_loop_unref(data.loop);
> -    if (data.error) {
> -        error_propagate(errp, data.error);
> -        object_unref(OBJECT(tioc));
> -        return NULL;
> -    }
> +    qemu_coroutine_yield();

Nice.

> +++ b/nbd/server.c
> @@ -668,16 +668,15 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
> uint16_t myflags,
>  
>  /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
>   * new channel for all further (now-encrypted) communication. */
> -static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,
> -                                                 Error **errp)
> +static QIOChannel coroutine_fn *nbd_co_negotiate_handle_starttls(

Awkward split of the return type; the coroutine_fn should instead be
placed after the *, as in:

block/mirror.c:static MirrorOp *coroutine_fn active_write_prepare(...

> +        NBDClient *client, Error **errp)
>  {
>      QIOChannel *ioc;
>      QIOChannelTLS *tioc;
> -    struct NBDTLSHandshakeData data = { 0 };

All uses of this type have been deleted; you should also remove it from
nbd-internal.h.


> @@ -1093,7 +1082,7 @@ static int nbd_negotiate_options(NBDClient *client, 
> uint16_t myflags,
>              return -EINVAL;
>          }
>  
> -        trace_nbd_negotiate_options_check_option(option,
> +        trace_nbd_co_negotiate_options_check_option(option,
>                                                   nbd_opt_lookup(option));

Indentation looks off.

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

signature.asc
Description: OpenPGP digital signature