On Wed, Jan 04, 2017 at 04:26:24PM -0500, Paolo Bonzini wrote:
> > > @@ -84,8 +83,8 @@ struct QIOChannel {
> > > +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.
Ok, that's a little surprising :-) Can you document on qio_channel_yield
that its permitted to yield and set an aio context while waiting.
> > > + 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.
Yep, it feels unlikely. Tht said, it looks similar to the case
where you have two coroutines using the same channel, and one
does a yield(G_IO_IN) and the other does a yield(G_IO_OUT) while
the first is still waiting, which feels like a more plausible
scenario that could actually happen. So perhaps we do need to
consider 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
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
