On 15/07/2015 22:14, Kevin Wolf wrote:
> > index 233d8f5..ae7c6cf 100644
> > --- a/aio-win32.c
> > +++ b/aio-win32.c
> > @@ -318,11 +313,11 @@ bool aio_poll(AioContext *ctx, bool blocking)
> > first = true;
> >
> > /* wait until next event */
> > - while (count > 0) {
> > + do {
> > HANDLE event;
> > int ret;
> >
> > - timeout = blocking
> > + timeout = blocking && !have_select_revents
> > ? qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)) : 0;
> > if (timeout) {
> > aio_context_release(ctx);
>
> Here I'm not sure why you switched to a do..while loop? It's not obvious
> to me how the change from aio_set_dispatching() to ctx->notify_me is
> related to this.
I sort of expected a reviewer to ask me about this change. I would have
explained this in the commit message, but it was already long enough. :)
The count on entry is never zero, because the ctx->notifier is always
registered.
I changed the while to do...while so that it's obvious that
ctx->notify_me is always decremented.
In retrospect I could have added simply
/* ctx->notifier is always registered. */
assert(count > 0);
above the "do".
> With this information, I understand that what has changed is that the
> return value of g_main_context_iteration() changes from true before this
> patch (had the aio_notify() from aio_set_fd_handler() pending) to false
> after the patch (aio_notify() doesn't inject an event any more).
>
> This should mean that like above we can assert that the first iteration
> returns false, i.e. reverse the assertion (and indeed, with this
> change the test still passes for me).
I was a bit undecided about this. In the end I decided that the calls
to aio_poll/g_main_context_iteration were just to put the AioContext in
a known state, and the assertions on the return value of g_assert were
not really important. For this reason, the while loop seemed to express
the intentions best, and I made it consistent between the AioContext and
GSource cases.
Paolo
