[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/4] nbd: do qemu_coroutine_yield during tls han
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH 3/4] nbd: do qemu_coroutine_yield during tls handshake |
Date: |
Tue, 19 Feb 2019 10:40:03 +0000 |
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 <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 ?
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