Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

["Daniel P. Berrange" <berrange@redhat.com>]

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 <pbonzini@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 <berrange@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 :|