> > @@ -84,8 +83,8 @@ struct QIOChannel {
> > unsigned int features; /* bitmask of QIOChannelFeatures */
> > char *name;
> > AioContext *ctx;
> > - QIOChannelRestart *read_coroutine;
> > - QIOChannelRestart *write_coroutine;
> > + Coroutine *read_coroutine;
> > + Coroutine *write_coroutine;
>
> Seems this ought to have been squashed into the previous patch
Yeah, sent this last thing before Christmas and it shows. :)
> > +static void qio_channel_set_fd_handlers(QIOChannel *ioc)
> > +{
> > + IOHandler *rd_handler = NULL, *wr_handler = NULL;
> > +
> > + if (ioc->read_coroutine) {
> > + rd_handler = qio_channel_restart_read;
> > + }
> > + if (ioc->write_coroutine) {
> > + rd_handler = qio_channel_restart_write;
> > + }
> > +
> > + qio_channel_set_fd_handler(ioc,
> > + ioc->ctx ? ioc->ctx :
> > iohandler_get_aio_context(),
> > + rd_handler, wr_handler, ioc);
> > +}
>
> ioc->read_coroutine & ioc->write_coroutine can only be non-NULL during
> a qio_channel_yield() caller. So it seems that calling
> qio_channel_set_fd_handlers() from the qio_channel_set_aio_context()
> method in the previous patch is not required, as those two callback
> pointers will always be NULL.
Not necessarily. You can have one coroutine calling qio_channel_yield(),
and then the non-coroutine code can call qio_channel_set_aio_context()
before the coroutine reenters.
This actually happens in the next patch. Where the NBD socket is quiescent
and no response is in flight, such as during a bdrv_drain_begin/end()
section, the "coroutine that receives NBD headers" has yielded. This
is also the time when set_aio_context can be called.
> > + if (condition == G_IO_IN) {
> > + ioc->read_coroutine = qemu_coroutine_self();
> > + } else if (condition == G_IO_OUT) {
> > + ioc->write_coroutine = qemu_coroutine_self();
> > + } else {
> > + abort();
> > + }
>
> Do we really need this to be an either/or/abort ? It looks like
> the qio_channel_set_fd_handlers() method is happy top have both
> read_coroutine & write_coroutine set.
The idea is that this would be called by a coroutine after a
recv or send that returns EAGAIN (with G_IO_IN for recv and
G_IO_OUT for send). If not exclusive, you'd have to check
for ioc->read_coroutine == ioc->write_coroutine in the handler.
Not a big deal, I can do it, but it adds an edge case and I
didn't see a use for it.
> If it does need to be exclusive though, can you update the API
> docs for this method to mention that.
Sure.
Thanks for the speedy review!
Paolo
