Re: [Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine

[Vladimir Sementsov-Ogievskiy]

12.02.2019 0:38, Eric Blake wrote:
> On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
>> As a first step to non-blocking negotiation, move it to coroutine.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   nbd/client.c | 123 +++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 109 insertions(+), 14 deletions(-)
>>
>> diff --git a/nbd/client.c b/nbd/client.c
>> index 10a52ad7d0..2ba2220a4a 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -859,13 +859,14 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
>>    *          2: server is newstyle, but lacks structured replies
>>    *          3: server is newstyle and set up for structured replies
>>    */
>> -static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>> -                               const char *hostname, QIOChannel **outioc,
>> -                               bool structured_reply, bool *zeroes,
>> -                               Error **errp)
>> +static int coroutine_fn nbd_co_start_negotiate(
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        QIOChannel **outioc, bool structured_reply, bool *zeroes, Error 
>> **errp)
>>   {
>>       uint64_t magic;
>>   
>> +    assert(qemu_in_coroutine());
>> +
> 
> Do we need this assert, since this function is static, and only called by:

OK, I think this can be dropped

> 
>>       trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
>>   
>>       if (zeroes) {
>> @@ -990,19 +991,20 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel 
>> *ioc, NBDExportInfo *info,
>>    * Returns: negative errno: failure talking to server
>>    *          0: server is connected
>>    */
>> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
>> -                          const char *hostname, QIOChannel **outioc,
>> -                          NBDExportInfo *info, Error **errp)
>> +static int coroutine_fn nbd_co_receive_negotiate(
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>>   {
>>       int result;
>>       bool zeroes;
>>       bool base_allocation = info->base_allocation;
>>   
>> +    assert(qemu_in_coroutine());
>>       assert(info->name);
>>       trace_nbd_receive_negotiate_name(info->name);
>>   
>> -    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
>> -                                 info->structured_reply, &zeroes, errp);
>> +    result = nbd_co_start_negotiate(ioc, tlscreds, hostname, outioc,
>> +                                   info->structured_reply, &zeroes, errp);
> 
> other functions in the same file that have also made the same assertion?
>   For that matter, does the assert() add any value over the fact that we
> already annotated things as coroutine_fn?
> 
> I know that at some point, there was a proposal on the list for having
> the compiler enforce coroutine_fn annotations (ensuring that a
> coroutine_fn only calls other coroutine_fns, and that coroutines can
> only be started on an entry point properly marked), but that's a
> different task for another day.
> 

I thought, that converting function to be "coroutine_fn" (not creating a new 
one)
is a good reason to add an assertion.

> 
>> +static int nbd_receive_common(CoroutineEntry *entry,
>> +                              NbdReceiveNegotiateCo *data)
>> +{
>> +    data->ret = -EINPROGRESS;
>> +
>> +    if (qemu_in_coroutine()) {
>> +        entry(data);
>> +    } else {
>> +        AioContext *ctx = qio_channel_get_attached_aio_context(data->ioc);
>> +        bool attach = !ctx;
> 
> There's where you used the function added in 1/4.
> 
>> +
>> +        if (attach) {
>> +            ctx = qemu_get_current_aio_context();
>> +            qio_channel_attach_aio_context(data->ioc, ctx);
>> +        }
>> +
>> +        qemu_aio_coroutine_enter(ctx, qemu_coroutine_create(entry, data));
>> +        AIO_WAIT_WHILE(ctx, data->ret == -EINPROGRESS);
>> +
>> +        if (attach) {
>> +            qio_channel_detach_aio_context(data->ioc);
>> +        }
>> +    }
>> +
>> +    return data->ret;
>> +}
> 
> Looks reasonable.
> 
>> +
>> +int nbd_receive_negotiate(
>> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
>> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>> +{
> 
> Why the reflowed parameter list, compared to its existing listing (as
> shown by the - lines where you added nbd_receive_co_negotiate)? That's
> only cosmetic, so I can live with it, but it seems like it makes the
> diff slightly larger.
> 

Hmm, maybe, will change it back.

-- 
Best regards,
Vladimir