On 09/03/2016 18:28, Daniel P. Berrange wrote: > From: Paolo Bonzini <pbonz...@redhat.com>
Reviewing my own patch looks weird. :) > On Win32 we cannot directly poll on socket handles. Instead we > create a Win32 event object and associate the socket handle with > the event. When the event signals readyness we then have to > use select to determine which events are ready. Creating Win32 > events is moderately heavyweight, so we don't want todo it > every time we create a GSource, so this associates a single > event with a QIOChannel. > > Signed-off-by: Daniel P. Berrange <berra...@redhat.com> > --- > include/io/channel.h | 3 ++ > io/channel-socket.c | 9 ++++ > io/channel-watch.c | 136 > ++++++++++++++++++++++++++++++++++++++++++++++++++- > io/channel.c | 14 ++++++ > 4 files changed, 161 insertions(+), 1 deletion(-) > > diff --git a/include/io/channel.h b/include/io/channel.h > index 0a1f1ce..20b973a 100644 > --- a/include/io/channel.h > +++ b/include/io/channel.h > @@ -78,6 +78,9 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc, > struct QIOChannel { > Object parent; > unsigned int features; /* bitmask of QIOChannelFeatures */ > +#ifdef _WIN32 > + HANDLE event; /* For use with GSource on Win23 */ Even s390 would have Win24 and Win31 but not Win23. :) > +#endif > }; > > /** > diff --git a/io/channel-socket.c b/io/channel-socket.c > index 6f7f594..ff49853 100644 > --- a/io/channel-socket.c > +++ b/io/channel-socket.c > @@ -55,6 +55,10 @@ qio_channel_socket_new(void) > ioc = QIO_CHANNEL(sioc); > ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > +#ifdef WIN32 > + ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > +#endif > + > trace_qio_channel_socket_new(sioc); > > return sioc; > @@ -341,6 +345,11 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > cioc->remoteAddrLen = sizeof(ioc->remoteAddr); > cioc->localAddrLen = sizeof(ioc->localAddr); > > +#ifdef WIN32 > + ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL); > +#endif QIO_CHANNEL(cioc)->event? > + WSAEventSelect(ssource->socket, NULL, 0); This should probably be moved in qio_channel_socket_finalize. > > + /* WSAEnumNetworkEvents is edge-triggered, so we need a separate > + * call to select to find which events are actually available. > + * However, if there were no reported events at the time of the last > + * call, and no new events since then, we know that the socket is > + * quiescent. > + */ > + if (!ssource->revents && !ev.lNetworkEvents) { > + return 0; > + } > + This is unfortunately unsafe, because: 1) WSAEventSelect clears all pending events (so the next WSAEnumNetworkEvents returns no FD_READ, unless you have read all data in the buffer with recv) 2) setting a socket to non-blocking should call WSAEventSelect (see below), but cannot e.g. set ->revents to ~0 for all existing GSource. It's probably possible to add a generation count or something like that, but for now please revert this part (I had made it a separate commit in my prototype because I wasn't sure if it was okay). > + ssource->condition = condition; > + ssource->socket = socket; > + ssource->revents = 0; > + ssource->fd.fd = (gintptr)ioc->event; > + ssource->fd.events = G_IO_IN; > + > + g_source_add_poll(source, &ssource->fd); > + WSAEventSelect(ssource->socket, ioc->event, > + FD_READ | FD_ACCEPT | FD_CLOSE | > + FD_CONNECT | FD_WRITE | FD_OOB); This should be moved where the socket is made non-blocking in qio_channel_socket_set_blocking (because qemu_qemu_set_nonblock also calls WSAEventSelect). It's probably worth adding a comment that qio_channel_socket_source_check only works in non-blocking mode for Windows. Thanks, Paolo