Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32
On Fri, Mar 11, 2016 at 11:51:29PM +, Andrew Baumann wrote: > Hi folks, > > > From: Paolo Bonzini [mailto:pbonz...@redhat.com] > > 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 :|
Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32
Hi folks, > From: Paolo Bonzini [mailto:pbonz...@redhat.com] > 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! 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? Cheers, Andrew
Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32
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. > > Patch 1 addresses Andrew's 2nd stated problem - handling of > getpeername() failures, by fixing errno handling on Win32. > > Patches 2-7 do some fixes in the test-io-channel-socket test > case so that it is able to run on Win32. > > Patches 8-12 are some fixes for the QIOChannel code > > Patch 13 is the big one that changes QIOChannelSocket so that > it uses a Win32 specific GSource implementation for creating > watches. This is the key fix for Andrew's 1st stated problem. > > At this point tests/test-io-channel-socket passes and > > qemu-system-x86_64.exe -serial tcp:127.0.0.1:9000,server,nowait -device > sga -display non > > works on win32 once more. > > Patches 14-16 are some cleanups to the chardev code to improve > its clarity. They are not required for fixing any real problem > > Patches 17-18 change the way we provide Win32 portability for > sockets APIs inside QEMU. These do fix a number of bugs in the > QEMU code related to mistaken use of errno instead of > socket_error(). None of these bugs appear to be critical issues. > > Based on this, I'm proposing 1-13 for QEMU 2.6 release as they > fix critical win32 bugs. > > Patches 14-18 can either be included in 2.6 or 2.7 - I'm > ambivalent on which, since they're cleanups / minor fixes. Thanks, please submit all of them in a pull request for 2.6. We can then clean up EAGAIN vs. EWOULDBLOCK and add a checkpatch rule to prevent further introduction of EWOULDBLOCK. Paolo
