[Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Stefan Weil]

It is broken since commit c619644067f98098dcdbc951e2dda79e97560afa.

Reported-by: Michael Fritscher <michael@fritscher.net>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Networking with QEMU for Windows is currently not usable,
see bug report https://bugs.launchpad.net/qemu/+bug/1569988.

With this patch, it seems to work again at least partially.
Michael Fritscher reported that it is still slow, so
more fixes might be needed.

Would it be better to add conditional compilation to
slirp/tcp_input.c again (then the changes would only
be for Windows, so no new risk for QEMU 2.6)?

Peter, I'd appreciate to get Windows networking fixed
for 2.6, so feel free to modify and apply this patch as
needed if time is too short for reviews and my pull request.

Regards,
Stefan

 slirp/slirp.h     | 5 -----
 slirp/tcp_input.c | 1 +
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/slirp/slirp.h b/slirp/slirp.h
index c99ebb9..203deec 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -347,9 +347,4 @@ struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
 #define max(x,y) ((x) > (y) ? (x) : (y))
 #endif
 
-#ifdef _WIN32
-#undef errno
-#define errno (WSAGetLastError())
-#endif
-
 #endif
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 5433e7f..e2b5d4e 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -659,6 +659,7 @@ findso:
 	  }
 
 	  if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
+              (errno != EAGAIN) &&
               (errno != EINPROGRESS) && (errno != EWOULDBLOCK)
           ) {
 	    uint8_t code;
-- 
2.1.4

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Michael Fritscher]

> It is broken since commit c619644067f98098dcdbc951e2dda79e97560afa.
>
> Reported-by: Michael Fritscher <michael@fritscher.net>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> Networking with QEMU for Windows is currently not usable,
> see bug report https://bugs.launchpad.net/qemu/+bug/1569988.
>
> With this patch, it seems to work again at least partially.
> Michael Fritscher reported that it is still slow, so
> more fixes might be needed.
>
> Would it be better to add conditional compilation to
> slirp/tcp_input.c again (then the changes would only
> be for Windows, so no new risk for QEMU 2.6)?
>
> Peter, I'd appreciate to get Windows networking fixed
> for 2.6, so feel free to modify and apply this patch as
> needed if time is too short for reviews and my pull request.
>
> Regards,
> Stefan
>
>  slirp/slirp.h     | 5 -----
>  slirp/tcp_input.c | 1 +
>  2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index c99ebb9..203deec 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -347,9 +347,4 @@ struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
>  #define max(x,y) ((x) > (y) ? (x) : (y))
>  #endif
>
> -#ifdef _WIN32
> -#undef errno
> -#define errno (WSAGetLastError())
> -#endif
> -
>  #endif
> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> index 5433e7f..e2b5d4e 100644
> --- a/slirp/tcp_input.c
> +++ b/slirp/tcp_input.c
> @@ -659,6 +659,7 @@ findso:
>  	  }
>
>  	  if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
> +              (errno != EAGAIN) &&
>                (errno != EINPROGRESS) && (errno != EWOULDBLOCK)
>            ) {
>  	    uint8_t code;

Hi,

I tested it, works again (albeit being slow) :-)
Many thanks!

Tested-by: Michael Fritscher <michael@fritscher.net>

Best regards,
Michael Fritscher

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Samuel Thibault]

Hello,

Stefan Weil, on Thu 14 Apr 2016 19:46:17 +0200, wrote:
> Michael Fritscher reported that it is still slow, so
> more fixes might be needed.

Michael, what do you mean by "slow", the bandwidth, or the time to
connect?  Does it help if you disable ipv6?

Samuel

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Michael Fritscher]

Hello Samuel,


> Michael, what do you mean by "slow", the bandwidth, or the time to
> connect?  Does it help if you disable ipv6?

@Ipv6: I'll test it tomorrow.

I tested with wget http://www.heise.de.
On the guest program point of view, it seems to hang at waiting at the
first patch of data after sending the request. (see the screenshot I
sent you directly).  After starting receiving, it seems as the data were
here instantly (a bit slowed down by the strace, but this was very much
faster than the wait for the beginning of the response).

Another thing: The tcpdump which was running within qemu dropped 30 out
of 83 packets... It was running on a Skylake based Xeon CPU with almost
3 GHz - so I think this shouldn't be a problem^^ I sent you the tcpdumps
directly as well.

I tested it on Windows 7 64 bit.

Best regards,
Michael

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Michael Fritscher]

Hello Samuel,
> @Ipv6: I'll test it tomorrow.

Interesting results:

On my native windows builds with msys2 and mingw64, a time wget
http://www.heise.de gives me (tested under native Win7)
real 0m24.081s
user 0m2.313s
sys 0m9.620s

or

real 0m52.250s
user 0m3.647s
sys 0m21.117s

The results are quite the same for IPv4+IPv6 or IPv4 only.

Interestingly, they are working fine under Ubuntu Wine!

Stefan Weil builds (he has made a RC3 build in which the fix is applied)
is a lot faster - tested under native Win7:

real 0m2.581s
user 0m0.227s
sys 0m0480s

My cross builds built with Ubuntu and mingw64 are ok as well (tested under
wine)! I can't test them under Windows, because I got problems with the
vnc viewer...


The qemu command line is
qemu-system-x86_64.exe -m 512 --cdrom
c:\Users\michaelfritscher\Downloads\linux\KNOPPIX_V7.4.2DVD-2014-09-28-DE.iso
-netdev user,id=mynet0,restrict=n -device e1000,netdev=mynet0

A simple openssl speed aes shows roughly the same numbers on both builds.

My configure command: ./configure --target-list=x86_64-softmmu
--enable-sdl --extra-cflags=-mthreads

So:

Stefan build  + Windows 7 64 = ok
Native MSYS build + Windows 7 64= slow
Native MSYS build + Ubuntu + Wine = ok
My MingW build + Ubuntu + Wine = ok
My MingW build + Windows 7 = (not testable)

You find my builds under
http://mifritscher.de/austausch/qemu/qemu_msys2.zip and
http://mifritscher.de/austausch/qemu/qemu-linux-mingw64.tar.gz .

Best regards,
Michael Fritscher

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Peter Maydell]

On 14 April 2016 at 18:46, Stefan Weil <sw@weilnetz.de> wrote:
> It is broken since commit c619644067f98098dcdbc951e2dda79e97560afa.
>
> Reported-by: Michael Fritscher <michael@fritscher.net>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> Networking with QEMU for Windows is currently not usable,
> see bug report https://bugs.launchpad.net/qemu/+bug/1569988.
>
> With this patch, it seems to work again at least partially.
> Michael Fritscher reported that it is still slow, so
> more fixes might be needed.
>
> Would it be better to add conditional compilation to
> slirp/tcp_input.c again (then the changes would only
> be for Windows, so no new risk for QEMU 2.6)?
>
> Peter, I'd appreciate to get Windows networking fixed
> for 2.6, so feel free to modify and apply this patch as
> needed if time is too short for reviews and my pull request.

We've just missed rc2, but we have until next week for rc3,
so I think we have enough time for code review before then.
I've listed this bug on the Planning wiki page as a reminder
to make sure it's fixed before I tag rc3.

> Regards,
> Stefan
>
>  slirp/slirp.h     | 5 -----
>  slirp/tcp_input.c | 1 +
>  2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index c99ebb9..203deec 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -347,9 +347,4 @@ struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
>  #define max(x,y) ((x) > (y) ? (x) : (y))
>  #endif
>
> -#ifdef _WIN32
> -#undef errno
> -#define errno (WSAGetLastError())
> -#endif
> -

This is the sort of ugliness it's good to see the back of :-)

>  #endif
> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> index 5433e7f..e2b5d4e 100644
> --- a/slirp/tcp_input.c
> +++ b/slirp/tcp_input.c
> @@ -659,6 +659,7 @@ findso:
>           }
>
>           if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
> +              (errno != EAGAIN) &&
>                (errno != EINPROGRESS) && (errno != EWOULDBLOCK)
>            ) {
>             uint8_t code;

This change is safe (at least behaviour-wise) for Linux, because
on Linux EWOULDBLOCK and EAGAIN are the same thing. And we
already have code in slirp.c that's doing something like
"if (errno == EAGAIN || errno == EWOULBLOCK || ...)" so I
don't expect gcc/clang to complain about duplicate if clauses.

So I guess you can have my
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though Dan's opinion would also be good as the author of the
original commit.

(Incidentally the original intention in commit c61964406
was clearly that callers should need to check only EAGAIN and
not EWOULDBLOCK, and for 2.7 we should look at whether we can
do that (ie whether all our host OSes really make them the
same value, as Linux does and the oslib-win32.c socket_error()
function does).)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Daniel P. Berrange]

On Thu, Apr 14, 2016 at 08:12:00PM +0100, Peter Maydell wrote:
> On 14 April 2016 at 18:46, Stefan Weil <sw@weilnetz.de> wrote:
> > It is broken since commit c619644067f98098dcdbc951e2dda79e97560afa.
> >
> > Reported-by: Michael Fritscher <michael@fritscher.net>
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> >
> > Networking with QEMU for Windows is currently not usable,
> > see bug report https://bugs.launchpad.net/qemu/+bug/1569988.
> >
> > With this patch, it seems to work again at least partially.
> > Michael Fritscher reported that it is still slow, so
> > more fixes might be needed.
> >
> > Would it be better to add conditional compilation to
> > slirp/tcp_input.c again (then the changes would only
> > be for Windows, so no new risk for QEMU 2.6)?
> >
> > Peter, I'd appreciate to get Windows networking fixed
> > for 2.6, so feel free to modify and apply this patch as
> > needed if time is too short for reviews and my pull request.
> 
> We've just missed rc2, but we have until next week for rc3,
> so I think we have enough time for code review before then.
> I've listed this bug on the Planning wiki page as a reminder
> to make sure it's fixed before I tag rc3.
> 
> > Regards,
> > Stefan
> >
> >  slirp/slirp.h     | 5 -----
> >  slirp/tcp_input.c | 1 +
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/slirp/slirp.h b/slirp/slirp.h
> > index c99ebb9..203deec 100644
> > --- a/slirp/slirp.h
> > +++ b/slirp/slirp.h
> > @@ -347,9 +347,4 @@ struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
> >  #define max(x,y) ((x) > (y) ? (x) : (y))
> >  #endif
> >
> > -#ifdef _WIN32
> > -#undef errno
> > -#define errno (WSAGetLastError())
> > -#endif
> > -
> 
> This is the sort of ugliness it's good to see the back of :-)
> 
> >  #endif
> > diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> > index 5433e7f..e2b5d4e 100644
> > --- a/slirp/tcp_input.c
> > +++ b/slirp/tcp_input.c
> > @@ -659,6 +659,7 @@ findso:
> >           }
> >
> >           if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
> > +              (errno != EAGAIN) &&
> >                (errno != EINPROGRESS) && (errno != EWOULDBLOCK)
> >            ) {
> >             uint8_t code;
> 
> This change is safe (at least behaviour-wise) for Linux, because
> on Linux EWOULDBLOCK and EAGAIN are the same thing. And we
> already have code in slirp.c that's doing something like
> "if (errno == EAGAIN || errno == EWOULBLOCK || ...)" so I
> don't expect gcc/clang to complain about duplicate if clauses.
> 
> So I guess you can have my
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> though Dan's opinion would also be good as the author of the
> original commit.
> 
> (Incidentally the original intention in commit c61964406
> was clearly that callers should need to check only EAGAIN and
> not EWOULDBLOCK, and for 2.7 we should look at whether we can
> do that (ie whether all our host OSes really make them the
> same value, as Linux does and the oslib-win32.c socket_error()
> function does).)

Fortunately someone has already created a giant table:

  http://www.ioplex.com/~miallen/errcmp.html

Aside from Windows, only OSF/1 has different value for EAGAIN & WOULDBLOCK
and IIUC we don't support QEMU on that platform, so I think we're fine.


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 for 2.6] wxx: Fix broken TCP networking (regression)

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

Am 15.04.2016 um 11:15 schrieb Daniel P. Berrange:
> On Thu, Apr 14, 2016 at 08:12:00PM +0100, Peter Maydell wrote:
[...]
>> (Incidentally the original intention in commit c61964406
>> was clearly that callers should need to check only EAGAIN and
>> not EWOULDBLOCK, and for 2.7 we should look at whether we can
>> do that (ie whether all our host OSes really make them the
>> same value, as Linux does and the oslib-win32.c socket_error()
>> function does).)
> 
> Fortunately someone has already created a giant table:
> 
>   http://www.ioplex.com/~miallen/errcmp.html
> 
> Aside from Windows, only OSF/1 has different value for EAGAIN & WOULDBLOCK
> and IIUC we don't support QEMU on that platform, so I think we're fine.


AIX 4.3,5.1 uses 11/54, HP-UX 11.22 uses 11/246 for EAGAIN/EWOULDBLOCK
according to that table.

Is this relevant for QEMU?



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Peter Maydell]

On 15 April 2016 at 17:56, Stefan Weil <sw@weilnetz.de> wrote:
> AIX 4.3,5.1 uses 11/54, HP-UX 11.22 uses 11/246 for EAGAIN/EWOULDBLOCK
> according to that table.
>
> Is this relevant for QEMU?

We still have support in configure for AIX. I have no idea when
anybody last tested it, so my money would be on it being bitrotted.
I don't think we've ever supported HP-UX.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Am 15.04.2016 um 18:59 schrieb Peter Maydell:
> On 15 April 2016 at 17:56, Stefan Weil <sw@weilnetz.de> wrote:
>> AIX 4.3,5.1 uses 11/54, HP-UX 11.22 uses 11/246 for EAGAIN/EWOULDBLOCK
>> according to that table.
>>
>> Is this relevant for QEMU?
> 
> We still have support in configure for AIX. I have no idea when
> anybody last tested it, so my money would be on it being bitrotted.
> I don't think we've ever supported HP-UX.
> 
> thanks
> -- PMM


To minimize any risks I did not change the reviewed patch
in the pull request which I have just sent.

As Daniel has pointed out there remains some work to be done after 2.6.

Thanks to all of you for reviews and feedback.

Stefan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression)

[Daniel P. Berrange]

On Thu, Apr 14, 2016 at 07:46:17PM +0200, Stefan Weil wrote:
> It is broken since commit c619644067f98098dcdbc951e2dda79e97560afa.
> 
> Reported-by: Michael Fritscher <michael@fritscher.net>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> Networking with QEMU for Windows is currently not usable,
> see bug report https://bugs.launchpad.net/qemu/+bug/1569988.
> 
> With this patch, it seems to work again at least partially.
> Michael Fritscher reported that it is still slow, so
> more fixes might be needed.
> 
> Would it be better to add conditional compilation to
> slirp/tcp_input.c again (then the changes would only
> be for Windows, so no new risk for QEMU 2.6)?
> 
> Peter, I'd appreciate to get Windows networking fixed
> for 2.6, so feel free to modify and apply this patch as
> needed if time is too short for reviews and my pull request.
> 
> Regards,
> Stefan
> 
>  slirp/slirp.h     | 5 -----
>  slirp/tcp_input.c | 1 +
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index c99ebb9..203deec 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -347,9 +347,4 @@ struct tcpcb *tcp_drop(struct tcpcb *tp, int err);
>  #define max(x,y) ((x) > (y) ? (x) : (y))
>  #endif
>  
> -#ifdef _WIN32
> -#undef errno
> -#define errno (WSAGetLastError())
> -#endif
> -
>  #endif
> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> index 5433e7f..e2b5d4e 100644
> --- a/slirp/tcp_input.c
> +++ b/slirp/tcp_input.c
> @@ -659,6 +659,7 @@ findso:
>  	  }
>  
>  	  if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
> +              (errno != EAGAIN) &&
>                (errno != EINPROGRESS) && (errno != EWOULDBLOCK)
>            ) {
>  	    uint8_t code;

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


Technically you can also kill that EWOULDBLOCK check there and in
other files since we gaurantee you'll always get EAGAIN now.


Unrelated to the problm you describe, I notice in socket.c there are
also a couple of places which call WSASetLastError which should be
removed, so it just sets errno unconditionally. These merely affect
error reporting quality though so not critical.


Also unrelated, we should probably kill the WSAStartup() call in slirp.c
because QEMU vl.c ensures that's called already

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