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 <vsement...@virtuozzo.com> >> --- >> 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