[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 u
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select |
Date: |
Thu, 10 Mar 2016 14:50:14 +0000 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
>
> 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 <address@hidden>
> > ---
> > 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 :|
- [Qemu-devel] [PATCH v1 09/21] io: add missing EWOULDBLOCK checks in Win32 I/O code paths, (continued)
- [Qemu-devel] [PATCH v1 09/21] io: add missing EWOULDBLOCK checks in Win32 I/O code paths, Daniel P. Berrange, 2016/03/09
- [Qemu-devel] [PATCH v1 10/21] io: pass HANDLE to g_source_add_poll on Win32, Daniel P. Berrange, 2016/03/09
- [Qemu-devel] [PATCH v1 11/21] io: introduce qio_channel_create_socket_watch, Daniel P. Berrange, 2016/03/09
- [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Daniel P. Berrange, 2016/03/09
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Paolo Bonzini, 2016/03/09
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Eric Blake, 2016/03/09
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Paolo Bonzini, 2016/03/09
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Daniel P. Berrange, 2016/03/10
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Paolo Bonzini, 2016/03/10
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select, Eric Blake, 2016/03/10
- Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select,
Daniel P. Berrange <=
[Qemu-devel] [PATCH v1 14/21] char: remove qemu_chr_finish_socket_connection method, Daniel P. Berrange, 2016/03/09
[Qemu-devel] [PATCH v1 13/21] char: ensure listener socket is in blocking mode when waiting, Daniel P. Berrange, 2016/03/09
[Qemu-devel] [PATCH v1 15/21] char: remove socket_try_connect method, Daniel P. Berrange, 2016/03/09
[Qemu-devel] [PATCH v1 17/21] osdep: add wrappers for socket functions, Daniel P. Berrange, 2016/03/09
[Qemu-devel] [PATCH v1 16/21] char: remove qemu_chr_open_socket_fd method, Daniel P. Berrange, 2016/03/09