On 05/01/2017 11:26, Daniel P. Berrange wrote:
>>>> + 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
Yes, it is possible to have both two conditions active at the same time
for two different coroutines and it does happen. As in the
set_aio_context case, NBD has G_IO_IN pretty much always active to poll
for responses---and in the meanwhile a coroutine might send a request
and activate G_IO_OUT polling. In this case, however, there would be
two qio_channel_yield calls.
The unlikely part is when a single coroutine wants to poll both conditions.
One improvement I can do here is to assert !ioc->read_coroutine if
condition is G_IO_IN and !ioc->write_coroutine if condition is G_IO_OUT.
If this is not respected, the coroutine that yielded first would never
be called back again!
In any case, I'm happy that overall the API and the implementation look
sane to you.
Paolo
