[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regress
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression) |
Date: |
Fri, 15 Apr 2016 10:15:16 +0100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 :|
Re: [Qemu-devel] [PATCH for 2.6] wxx: Fix broken TCP networking (regression), Daniel P. Berrange, 2016/04/15