| Message ID | 1335565745-22708-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 27, 2012 at 22:29, Andreas Färber <afaerber@suse.de> wrote: > From: Andreas Färber <andreas.faerber@web.de> > > Commit b72210568ef0c0fb141a01cffb71a09c4efa0364 (slirp: clean up > conflicts with system headers) enclosed TCPOLEN_MAXSEG with an #ifdef > TCPOPT_EOL. This broke the build on illumos, which has TCPOPT_* > but not TCPOLEN_*. > > Move them to their own #ifdef TCPOLEN_MAXSEG section to remedy this. This is just a band aid for the general problem, please introduce QEMU_ or SLIRP_ prefix so we that avoid system headers and the #ifdeffery. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Andreas Färber <andreas.faerber@web.de> > --- > slirp/tcp.h | 13 ++++++++----- > 1 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/slirp/tcp.h b/slirp/tcp.h > index 8299603..2e2b403 100644 > --- a/slirp/tcp.h > +++ b/slirp/tcp.h > @@ -79,20 +79,23 @@ struct tcphdr { > #define TCPOPT_EOL 0 > #define TCPOPT_NOP 1 > #define TCPOPT_MAXSEG 2 > -#define TCPOLEN_MAXSEG 4 > #define TCPOPT_WINDOW 3 > -#define TCPOLEN_WINDOW 3 > #define TCPOPT_SACK_PERMITTED 4 /* Experimental */ > -#define TCPOLEN_SACK_PERMITTED 2 > #define TCPOPT_SACK 5 /* Experimental */ > #define TCPOPT_TIMESTAMP 8 > -#define TCPOLEN_TIMESTAMP 10 > -#define TCPOLEN_TSTAMP_APPA (TCPOLEN_TIMESTAMP+2) /* appendix A */ > > #define TCPOPT_TSTAMP_HDR \ > (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP) > #endif > > +#ifndef TCPOLEN_MAXSEG > +#define TCPOLEN_MAXSEG 4 > +#define TCPOLEN_WINDOW 3 > +#define TCPOLEN_SACK_PERMITTED 2 > +#define TCPOLEN_TIMESTAMP 10 > +#define TCPOLEN_TSTAMP_APPA (TCPOLEN_TIMESTAMP+2) /* appendix A */ > +#endif > + > /* > * Default maximum segment size for TCP. > * With an IP MSS of 576, this is 536, > -- > 1.7.7 > >
Am 28.04.2012 10:18, schrieb Blue Swirl: > On Fri, Apr 27, 2012 at 22:29, Andreas Färber <afaerber@suse.de> wrote: >> From: Andreas Färber <andreas.faerber@web.de> >> >> Commit b72210568ef0c0fb141a01cffb71a09c4efa0364 (slirp: clean up >> conflicts with system headers) enclosed TCPOLEN_MAXSEG with an #ifdef >> TCPOPT_EOL. This broke the build on illumos, which has TCPOPT_* >> but not TCPOLEN_*. >> >> Move them to their own #ifdef TCPOLEN_MAXSEG section to remedy this. > > This is just a band aid for the general problem, please introduce > QEMU_ or SLIRP_ prefix so we that avoid system headers and the > #ifdeffery. This is a fix for a build regression introduced by Paolo. Do you seriously expect me to rewrite the whole of SLIRP on the eve of 1.1 Hard Freeze? A much easier solution would be to revert Paolo's commit. Andreas
On Sat, Apr 28, 2012 at 11:11, Andreas Färber <afaerber@suse.de> wrote: > Am 28.04.2012 10:18, schrieb Blue Swirl: >> On Fri, Apr 27, 2012 at 22:29, Andreas Färber <afaerber@suse.de> wrote: >>> From: Andreas Färber <andreas.faerber@web.de> >>> >>> Commit b72210568ef0c0fb141a01cffb71a09c4efa0364 (slirp: clean up >>> conflicts with system headers) enclosed TCPOLEN_MAXSEG with an #ifdef >>> TCPOPT_EOL. This broke the build on illumos, which has TCPOPT_* >>> but not TCPOLEN_*. >>> >>> Move them to their own #ifdef TCPOLEN_MAXSEG section to remedy this. >> >> This is just a band aid for the general problem, please introduce >> QEMU_ or SLIRP_ prefix so we that avoid system headers and the >> #ifdeffery. > > This is a fix for a build regression introduced by Paolo. Do you > seriously expect me to rewrite the whole of SLIRP on the eve of 1.1 Hard > Freeze? It would be The Right Fix and for most part, easily reviewable mechanical conversion. But it could be done later and only apply it to 1.1 if there are still problems with other OS headers. > > A much easier solution would be to revert Paolo's commit. But that would leave Illumos broken. I'd rather apply this one. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> But that would leave Illumos broken. I'd rather apply this one.
Please do. The right solution for 1.2 is to understand the places in which
slirp cannot use netinet/tcp.h constants (the MSS for example), and
otherwise use that header.
Paolo
Am 02.05.2012 15:24, schrieb Paolo Bonzini: >> But that would leave Illumos broken. For the record, reverting would leave whatever Paolo was fixing broken but would restore Illumos. > I'd rather apply this one. > > Please do. The right solution for 1.2 is to understand the places in which > slirp cannot use netinet/tcp.h constants (the MSS for example), and > otherwise use that header. Jan, should I prepare a mechanical prefix addition as suggested by Blue? Should we apply this ugly-but-least-intrusive build fix? Or do you have any other preference or idea? Andreas
On 2012-05-02 10:40, Andreas Färber wrote: > Am 02.05.2012 15:24, schrieb Paolo Bonzini: >>> But that would leave Illumos broken. > > For the record, reverting would leave whatever Paolo was fixing broken > but would restore Illumos. > >> I'd rather apply this one. >> >> Please do. The right solution for 1.2 is to understand the places in which >> slirp cannot use netinet/tcp.h constants (the MSS for example), and >> otherwise use that header. > > Jan, should I prepare a mechanical prefix addition as suggested by Blue? > Should we apply this ugly-but-least-intrusive build fix? Or do you have > any other preference or idea? I'm fine with a minimal fix like you suggested. The renaming should probably be applied to a broader scope. So it's an exercise to be done systematically and without any hurry. Jan
Am 28.04.2012 00:29, schrieb Andreas Färber: > From: Andreas Färber <andreas.faerber@web.de> > > Commit b72210568ef0c0fb141a01cffb71a09c4efa0364 (slirp: clean up > conflicts with system headers) enclosed TCPOLEN_MAXSEG with an #ifdef > TCPOPT_EOL. This broke the build on illumos, which has TCPOPT_* > but not TCPOLEN_*. > > Move them to their own #ifdef TCPOLEN_MAXSEG section to remedy this. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Andreas Färber <andreas.faerber@web.de> Ping! It seems this patch has not been applied yet despite agreement between Blue and Jan that we should apply this least-invasive version for 1.1. Please apply / include in a PULL for rc4. Andreas > --- > slirp/tcp.h | 13 ++++++++----- > 1 files changed, 8 insertions(+), 5 deletions(-) > > diff --git a/slirp/tcp.h b/slirp/tcp.h > index 8299603..2e2b403 100644 > --- a/slirp/tcp.h > +++ b/slirp/tcp.h > @@ -79,20 +79,23 @@ struct tcphdr { > #define TCPOPT_EOL 0 > #define TCPOPT_NOP 1 > #define TCPOPT_MAXSEG 2 > -#define TCPOLEN_MAXSEG 4 > #define TCPOPT_WINDOW 3 > -#define TCPOLEN_WINDOW 3 > #define TCPOPT_SACK_PERMITTED 4 /* Experimental */ > -#define TCPOLEN_SACK_PERMITTED 2 > #define TCPOPT_SACK 5 /* Experimental */ > #define TCPOPT_TIMESTAMP 8 > -#define TCPOLEN_TIMESTAMP 10 > -#define TCPOLEN_TSTAMP_APPA (TCPOLEN_TIMESTAMP+2) /* appendix A */ > > #define TCPOPT_TSTAMP_HDR \ > (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP) > #endif > > +#ifndef TCPOLEN_MAXSEG > +#define TCPOLEN_MAXSEG 4 > +#define TCPOLEN_WINDOW 3 > +#define TCPOLEN_SACK_PERMITTED 2 > +#define TCPOLEN_TIMESTAMP 10 > +#define TCPOLEN_TSTAMP_APPA (TCPOLEN_TIMESTAMP+2) /* appendix A */ > +#endif > + > /* > * Default maximum segment size for TCP. > * With an IP MSS of 576, this is 536,
On 2012-05-27 19:43, Andreas Färber wrote: > Am 28.04.2012 00:29, schrieb Andreas Färber: >> From: Andreas Färber <andreas.faerber@web.de> >> >> Commit b72210568ef0c0fb141a01cffb71a09c4efa0364 (slirp: clean up >> conflicts with system headers) enclosed TCPOLEN_MAXSEG with an #ifdef >> TCPOPT_EOL. This broke the build on illumos, which has TCPOPT_* >> but not TCPOLEN_*. >> >> Move them to their own #ifdef TCPOLEN_MAXSEG section to remedy this. >> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Andreas Färber <andreas.faerber@web.de> > > Ping! It seems this patch has not been applied yet despite agreement > between Blue and Jan that we should apply this least-invasive version > for 1.1. Please apply / include in a PULL for rc4. Thanks, queued for 1.1. I'll send a pull as soon as the other small build fixes are available. Jan
diff --git a/slirp/tcp.h b/slirp/tcp.h index 8299603..2e2b403 100644 --- a/slirp/tcp.h +++ b/slirp/tcp.h @@ -79,20 +79,23 @@ struct tcphdr { #define TCPOPT_EOL 0 #define TCPOPT_NOP 1 #define TCPOPT_MAXSEG 2 -#define TCPOLEN_MAXSEG 4 #define TCPOPT_WINDOW 3 -#define TCPOLEN_WINDOW 3 #define TCPOPT_SACK_PERMITTED 4 /* Experimental */ -#define TCPOLEN_SACK_PERMITTED 2 #define TCPOPT_SACK 5 /* Experimental */ #define TCPOPT_TIMESTAMP 8 -#define TCPOLEN_TIMESTAMP 10 -#define TCPOLEN_TSTAMP_APPA (TCPOLEN_TIMESTAMP+2) /* appendix A */ #define TCPOPT_TSTAMP_HDR \ (TCPOPT_NOP<<24|TCPOPT_NOP<<16|TCPOPT_TIMESTAMP<<8|TCPOLEN_TIMESTAMP) #endif +#ifndef TCPOLEN_MAXSEG +#define TCPOLEN_MAXSEG 4 +#define TCPOLEN_WINDOW 3 +#define TCPOLEN_SACK_PERMITTED 2 +#define TCPOLEN_TIMESTAMP 10 +#define TCPOLEN_TSTAMP_APPA (TCPOLEN_TIMESTAMP+2) /* appendix A */ +#endif + /* * Default maximum segment size for TCP. * With an IP MSS of 576, this is 536,