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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <vsementsov@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