Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32

[Daniel P. Berrange]

On Fri, Mar 11, 2016 at 11:51:29PM +0000, Andrew Baumann wrote:
> Hi folks,
> 
> > From: Paolo Bonzini [mailto:address@hidden
> > Sent: Thursday, 10 March 2016 9:37 AM
> > 
> > On 10/03/2016 18:26, Daniel P. Berrange wrote:
> > > This series started out as an attempt to fix the Win32 problems
> > > identified by Andrew Baumann
> > >
> > >    https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01343.html
> > >
> > > It turned into a significantly larger cleanup of some chardev
> > > and osdep win32 portability code.
> [...]
> 
> Sorry for chiming in a bit late here. I've tested these patches
> (the complete set, not individually), and they do appear to fix my
> immediate problem: socket char devices now work again. So thank you!

Thanks for confirming this, these patches have now merged into
git msater.

> However, I'm now seeing a problem I don't believe we had before:
> very slow responses to GDB commands. From looking at a packet
> capture (using a localhost tcp socket between qemu and my gdb
> client), it seems that a couple of operations will go through
> just fine, and then there is a 1 second delay between my client's
> request and qemu's response. After fiddling with poll timeouts,
> it became clear that we were noticing the socket events when
> waking up from the poll, but the events themselves were still
> not waking us. It turns out that we were not calling WSAEventSelect
> on the accept path. At least, the following patch fixed the
> problem for me:
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 3bf30b5..c1be622 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3047,6 +3047,7 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
>          return TRUE;
>      }
> 
> +    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>      tcp_chr_new_client(chr, sioc);
> 
>      object_unref(OBJECT(sioc));
> 
> However, I'd note that both callers of tcp_chr_new_client()
> make the same call to set blocking to false immediately before
> calling tcp_chr_new_client(). Furthermore, the doc comment for
> qio_channel_set_blocking() appears to suggest that non-blocking
> mode is the default. If that's true, maybe you don't even want
> to rely on the caller explicitly setting blocking to false?

No, the docs don't intend to suggest that - the default is in
fact blocking mode, so its correct to place it into nonblocking
mode explicitly.

I think I didn't notice the problem you describe because my original
patch series had us call WSAEventSelect when creating the watch. This
indirectly puts Win32 sockets into non-blocking mode. The patches which
just merged however no longer call WSAEventSelect when creating the
watch, instead requiring the caller to explicitly set the socket into
non-blocking mode. So I think your suggested addition here is probably
the right way to address this. I'll investigate and respond with a
followup patch as needed.

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 :|