12.02.2019 0:55, Eric Blake wrote: > 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 <vsement...@virtuozzo.com> >> --- >> 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 ?
hmm, yes. > >> + 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? I think, not. It still may be called from non-coroutine context. > >> 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: Oops, agree. > > 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. OK > > >> @@ -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. > -- Best regards, Vladimir