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

[Peter Maydell]

On 14 April 2016 at 18:46, Stefan Weil <address@hidden> wrote:
> It is broken since commit c619644067f98098dcdbc951e2dda79e97560afa.
>
> Reported-by: Michael Fritscher <address@hidden>
> Signed-off-by: Stefan Weil <address@hidden>
> ---
>
> 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 <address@hidden>

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