On Wed, Mar 09, 2016 at 06:47:29PM +0100, Paolo Bonzini wrote: > 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/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. Yes, moved that. > > + /* 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). Ok, removing this chunk > > > + 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). Ok, I've moved that too. > It's probably worth adding a comment that > qio_channel_socket_source_check only works in non-blocking mode for Windows. And added this. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|