Patchwork slirp: Untangle TCPOLEN_* from TCPOPT_*

login
register
mail settings
Submitter Andreas Färber
Date April 27, 2012, 10:29 p.m.
Message ID <1335565745-22708-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/155601/
State New
Headers show

Comments

Andreas Färber - April 27, 2012, 10:29 p.m.
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>
---
 slirp/tcp.h |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
Blue Swirl - April 28, 2012, 8:18 a.m.
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
>
>
Andreas Färber - April 28, 2012, 11:11 a.m.
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
Blue Swirl - April 28, 2012, 11:23 a.m.
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
Paolo Bonzini - May 2, 2012, 1:24 p.m.
> 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
Andreas Färber - May 2, 2012, 1:40 p.m.
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
Jan Kiszka - May 2, 2012, 1:48 p.m.
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
Andreas Färber - May 27, 2012, 5:43 p.m.
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,
Jan Kiszka - May 28, 2012, 11:46 a.m.
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

Patch

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,