From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41347) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adiCv-0007Xz-Ts for qemu-devel@nongnu.org; Wed, 09 Mar 2016 12:47:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adiCr-0003Ee-Qj for qemu-devel@nongnu.org; Wed, 09 Mar 2016 12:47:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59293) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adiCr-0003E8-JC for qemu-devel@nongnu.org; Wed, 09 Mar 2016 12:47:33 -0500 References: <1457544504-8548-1-git-send-email-berrange@redhat.com> <1457544504-8548-13-git-send-email-berrange@redhat.com> From: Paolo Bonzini Message-ID: <56E061B1.9040706@redhat.com> Date: Wed, 9 Mar 2016 18:47:29 +0100 MIME-Version: 1.0 In-Reply-To: <1457544504-8548-13-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Stefan Weil , Andrew Baumann 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. >=20 > 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(-) >=20 > 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 > }; > =20 > /** > 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 =3D QIO_CHANNEL(sioc); > ioc->features |=3D (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > =20 > +#ifdef WIN32 > + ioc->event =3D CreateEvent(NULL, FALSE, FALSE, NULL); > +#endif > + > trace_qio_channel_socket_new(sioc); > =20 > return sioc; > @@ -341,6 +345,11 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > cioc->remoteAddrLen =3D sizeof(ioc->remoteAddr); > cioc->localAddrLen =3D sizeof(ioc->localAddr); > =20 > +#ifdef WIN32 > + ((QIOChannel *)cioc)->event =3D CreateEvent(NULL, FALSE, FALSE, NU= LL); > +#endif QIO_CHANNEL(cioc)->event? > + WSAEventSelect(ssource->socket, NULL, 0); This should probably be moved in qio_channel_socket_finalize. >=20 > + /* 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 la= st > + * 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 =3D condition; > + ssource->socket =3D socket; > + ssource->revents =3D 0; > + ssource->fd.fd =3D (gintptr)ioc->event; > + ssource->fd.events =3D 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 Windo= ws. Thanks, Paolo