From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51047) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ae1ux-0001Tj-LK for qemu-devel@nongnu.org; Thu, 10 Mar 2016 09:50:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ae1ut-0008Uv-89 for qemu-devel@nongnu.org; Thu, 10 Mar 2016 09:50:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60960) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ae1us-0008UY-PL for qemu-devel@nongnu.org; Thu, 10 Mar 2016 09:50:18 -0500 Date: Thu, 10 Mar 2016 14:50:14 +0000 From: "Daniel P. Berrange" Message-ID: <20160310145014.GO25607@redhat.com> References: <1457544504-8548-1-git-send-email-berrange@redhat.com> <1457544504-8548-13-git-send-email-berrange@redhat.com> <56E061B1.9040706@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56E061B1.9040706@redhat.com> Subject: Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Stefan Weil , qemu-devel@nongnu.org, Andrew Baumann 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 > > 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 > > --- > > 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 :|