| Message ID | 1460655977-436-1-git-send-email-sw@weilnetz.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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
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
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
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
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
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
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
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?
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
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
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;
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(-)