comment about struct tcp_tw_bucket in struct sock

comment about struct tcp_tw_bucket in struct sock

[Olaf Hering]

Hi,

we just hit this one. struct tcp_tw_bucket and stuct sock must match.

Please apply.



--- linux-2.4/include/net/sock.h	2003-04-24 17:11:20.000000000 +0200
+++ linux-2.4.21-rc2/include/net/sock.h	2003-05-13 23:00:32.000000000 +0200
@@ -498,6 +498,10 @@ do {	spin_lock_init(&((__sk)->lock.slock
 } while(0)
 
 struct sock {
+	/* These _must_ match the beginning of struct tcp_tw_bucket precisely.
+	 * XXX Yes I know this is gross, but I'd have to edit every single
+	 * XXX networking file if I created a "struct sock_header". -DaveM
+	 */
 	/* Socket demultiplex comparisons on incoming packets. */
 	__u32			daddr;		/* Foreign IPv4 addr			*/
 	__u32			rcv_saddr;	/* Bound local IPv4 addr		*/

-- 
USB is for mice, FireWire is for men!

Re: comment about struct tcp_tw_bucket in struct sock

[David S. Miller]

   From: Olaf Hering <olh@suse.de>
   Date: Tue, 13 May 2003 23:05:41 +0200

   we just hit this one. struct tcp_tw_bucket and stuct sock must match.
   
   Please apply.

It's documented in tcp.h already.

Re: comment about struct tcp_tw_bucket in struct sock

[Arnaldo Carvalho de Melo]

Em Tue, May 13, 2003 at 04:31:50PM -0700, David S. Miller escreveu:
>    From: Olaf Hering <olh@suse.de>
>    Date: Tue, 13 May 2003 23:05:41 +0200
> 
>    we just hit this one. struct tcp_tw_bucket and stuct sock must match.
>    
>    Please apply.
> 
> It's documented in tcp.h already.

Yes, and in 2.5 the situation is a bit better, I still have an itching to
revisit this at some point, together with struct open_request, etc 8)

- Arnaldo

Re: comment about struct tcp_tw_bucket in struct sock

[Olaf Hering]

 On Tue, May 13, David S. Miller wrote:

>    From: Olaf Hering <olh@suse.de>
>    Date: Tue, 13 May 2003 23:05:41 +0200
> 
>    we just hit this one. struct tcp_tw_bucket and stuct sock must match.
>    
>    Please apply.
> 
> It's documented in tcp.h already.

True, but not in struct sock.


-- 
USB is for mice, FireWire is for men!

Re: comment about struct tcp_tw_bucket in struct sock

[Andi Kleen]

On Tue, May 13, 2003 at 04:31:50PM -0700, David S. Miller wrote:
>    From: Olaf Hering <olh@suse.de>
>    Date: Tue, 13 May 2003 23:05:41 +0200
> 
>    we just hit this one. struct tcp_tw_bucket and stuct sock must match.
>    
>    Please apply.
> 
> It's documented in tcp.h already.

Just not everybody changing sock.h also reads tcp.h :-(

-Andi

Re: comment about struct tcp_tw_bucket in struct sock

[David S. Miller]

   From: Olaf Hering <olh@suse.de>
   Date: Wed, 14 May 2003 10:00:43 +0200

    On Tue, May 13, David S. Miller wrote:
   
   > It's documented in tcp.h already.
   
   True, but not in struct sock.

When you edit the layout of struct sock you assume
the full responsibility that you might be breaking
something that uses struct sock.  This includes
third party modules.

You shouldn't be making changing to it lightly.  How this made TCP
blow up is the negative feedback you get for trying to change struct
sock ;-)

Re: comment about struct tcp_tw_bucket in struct sock

[David S. Miller]

   From: Andi Kleen <ak@suse.de>
   Date: Wed, 14 May 2003 10:32:36 +0200

   On Tue, May 13, 2003 at 04:31:50PM -0700, David S. Miller wrote:
   > It's documented in tcp.h already.
   
   Just not everybody changing sock.h also reads tcp.h :-(

You assume that protocols in the tree are the only thing
that might break if you edit struct sock.

Re: comment about struct tcp_tw_bucket in struct sock

[Andi Kleen]

On Wed, May 14, 2003 at 12:18:06PM -0700, David S. Miller wrote:
>    From: Andi Kleen <ak@suse.de>
>    Date: Wed, 14 May 2003 10:32:36 +0200
> 
>    On Tue, May 13, 2003 at 04:31:50PM -0700, David S. Miller wrote:
>    > It's documented in tcp.h already.
>    
>    Just not everybody changing sock.h also reads tcp.h :-(
> 
> You assume that protocols in the tree are the only thing
> that might break if you edit struct sock.

I'm not assuming anything and didn't even edit struct sock, just pointing 
out that such a fragile hack as the current tw bucket is needs an explicit 
comment on both places.

Best would be to bite the bullet and give them a common structure.

-Andi

Re: comment about struct tcp_tw_bucket in struct sock

[Andi Kleen]

On Wed, May 14, 2003 at 12:17:27PM -0700, David S. Miller wrote:
> When you edit the layout of struct sock you assume
> the full responsibility that you might be breaking
> something that uses struct sock.  This includes
> third party modules.

What third party modules are you refering too ? 

I know openafs did or still does the cardinal sin of duplicating struct inode
and TCP has this hack too since ages, but other than that I hope 
"cut'n'paste" usage of structures is not common.

-Andi

Re: comment about struct tcp_tw_bucket in struct sock

[David S. Miller]

   From: Andi Kleen <ak@suse.de>
   Date: Wed, 14 May 2003 21:23:05 +0200

   What third party modules are you refering too ? 
   
We don't know, and that's precisely my point.

Re: comment about struct tcp_tw_bucket in struct sock

[Andi Kleen]

On Wed, May 14, 2003 at 12:34:21PM -0700, David S. Miller wrote:
>    From: Andi Kleen <ak@suse.de>
>    Date: Wed, 14 May 2003 21:23:05 +0200
> 
>    What third party modules are you refering too ? 
>    
> We don't know, and that's precisely my point.

When they cut'n'paste structures they're completely on their own ...

I hope you will never let stop such an omnious third party module
you from doing any bug fix or optimization.

-Andi

Re: comment about struct tcp_tw_bucket in struct sock

[David S. Miller]

   From: Andi Kleen <ak@suse.de>
   Date: Wed, 14 May 2003 21:57:31 +0200

   When they cut'n'paste structures they're completely on their own ...
   
Depends upon who's willing to support them.

   I hope you will never let stop such an omnious third party module
   you from doing any bug fix or optimization.
   
Aha, you're right and _I_ don't need to worry about this.  I'm merely
saying some vendor's _might_. :-)

But see, since it only matters in the main kernel, every attempt to
change the layout of struct sock will go through me before it hits the
vanilla tree, so as a result I'll catch such problems and we thus have
no problems :-)

You can't have it both ways Andi, either we care about arbitrary third
parties do or we don't.

Re: comment about struct tcp_tw_bucket in struct sock

[Arnaldo Carvalho de Melo]

Em Wed, May 14, 2003 at 09:21:17PM +0200, Andi Kleen escreveu:
> On Wed, May 14, 2003 at 12:18:06PM -0700, David S. Miller wrote:
> >    From: Andi Kleen <ak@suse.de>
> >    Date: Wed, 14 May 2003 10:32:36 +0200
> > 
> >    On Tue, May 13, 2003 at 04:31:50PM -0700, David S. Miller wrote:
> >    > It's documented in tcp.h already.
> >    
> >    Just not everybody changing sock.h also reads tcp.h :-(
> > 
> > You assume that protocols in the tree are the only thing
> > that might break if you edit struct sock.
> 
> I'm not assuming anything and didn't even edit struct sock, just pointing 
> out that such a fragile hack as the current tw bucket is needs an explicit 
> comment on both places.
> 
> Best would be to bite the bullet and give them a common structure.

Ah if we could just use a unnamed struct... 8)

- Arnaldo

Re: comment about struct tcp_tw_bucket in struct sock

[Andi Kleen]

> Ah if we could just use a unnamed struct... 8)

Don't worry. Sometime someone will find a gcc 2.95 bug that prevents
compilation of some new kernel code and is too hard to work around. Once
that happens everybody will move to 3.2+ for the kernel (I don't think
3.0 is used anymore) 

-Andi