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 <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).)

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