code review 140640045: go.net/ipv4: restructure sticky socket option handling

go.net/ipv4: restructure sticky socket option handling

This CL chops existing sticky socket option handlers and puts them
into platform dependent sticky socket option binding table for
supporting multicast features such as source filtering for any-source
multicast, source-specific multicast.

Also adds tiny syscall shims to help to support solaris, to improve
existing platform support.

[mikio, 2014-09-25 01:29:29]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.net

[mikio, 2014-09-27 00:42:38]

ping

[iant, 2014-09-27 00:58:08]

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go
File ipv4/sockopt_stub.go (right):

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go#ne...
ipv4/sockopt_stub.go:12: name int
I suggest that you start these constants at 1 so that 0 is an invalid number. 
Then you don't have to initialize every field in the arrays, just the valid
ones.

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go#ne...
ipv4/sockopt_stub.go:13: get  func(int, int) (interface{}, error)
It seems to me that all that matters is the type.  Suppose you have a type
constant here, and then call some function to do the actual getting and setting.

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go#ne...
ipv4/sockopt_stub.go:17: func (opt *sockOpt) getInt(fd int) (int, error) {
Why are these methods on sockOpt?

https://codereview.appspot.com/140640045/diff/1910001/ipv4/thunk_linux_386.s
File ipv4/thunk_linux_386.s (right):

https://codereview.appspot.com/140640045/diff/1910001/ipv4/thunk_linux_386.s#...
ipv4/thunk_linux_386.s:9: TEXT	·socketcall(SB),NOSPLIT,$0-36
I don't see any difference between socketcall in Go 1.2 and Go 1.4; why does 40
change to 36?

Is it really worth using a different file just to say NOSPLIT instead of 4?

[mikio, 2014-09-27 01:48:49]

thanks.

On Sat, Sep 27, 2014 at 9:58 AM,  <iant@golang.org> wrote:

> ipv4/thunk_linux_386.s:9: TEXT  ·socketcall(SB),NOSPLIT,$0-36
> I don't see any difference between socketcall in Go 1.2 and Go 1.4; why
> does 40 change to 36?

it's just a copy & paste from go1.2/1.3 syscall. do you prefer to adjust it?

> Is it really worth using a different file just to say NOSPLIT instead of
> 4?

for now i have no plan to drop go1.2/1.3 support.

[mikio, 2014-09-27 22:06:05]

ptal

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go
File ipv4/sockopt_stub.go (right):

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go#ne...
ipv4/sockopt_stub.go:12: name int
On 2014/09/27 00:58:08, iant wrote:
> I suggest that you start these constants at 1 so that 0 is an invalid number. 
> Then you don't have to initialize every field in the arrays, just the valid
> ones.

Done.

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go#ne...
ipv4/sockopt_stub.go:13: get  func(int, int) (interface{}, error)
On 2014/09/27 00:58:08, iant wrote:
> It seems to me that all that matters is the type.  Suppose you have a type
> constant here, and then call some function to do the actual getting and
setting.

Done.

https://codereview.appspot.com/140640045/diff/1910001/ipv4/sockopt_stub.go#ne...
ipv4/sockopt_stub.go:17: func (opt *sockOpt) getInt(fd int) (int, error) {
dropped methods on sockOpt

https://codereview.appspot.com/140640045/diff/1910001/ipv4/thunk_linux_386.s
File ipv4/thunk_linux_386.s (right):

https://codereview.appspot.com/140640045/diff/1910001/ipv4/thunk_linux_386.s#...
ipv4/thunk_linux_386.s:9: TEXT	·socketcall(SB),NOSPLIT,$0-36
> I don't see any difference between socketcall in Go 1.2 and Go 1.4; why does
40
> change to 36?

from syscall/asm_linux_386.s: TEXT ·socketcall(SB),NOSPLIT,$0-36

> Is it really worth using a different file just to say NOSPLIT instead of 4?

unlike go1.4, go1.2/1.3 don't expose visibility of textflag.h.

[mikio, 2014-09-27 22:11:06]

> from syscall/asm_linux_386.s: TEXT ·socketcall(SB),NOSPLIT,$0-36

hands slipped.

go1.2: TEXT ·socketcall(SB),NOSPLIT,$0-40
go1.3: TEXT ·socketcall(SB),NOSPLIT,$0-36
go1.4: TEXT ·socketcall(SB),NOSPLIT,$0-36

[mikio, 2014-09-30 00:20:01]

ptal

adjusted stack size for go1.2/13 assembly code

[iant, 2014-09-30 14:17:24]

Is there any way this CL can be made smaller?  It's very discouraging to look at
it.

[iant, 2014-09-30 14:28:05]

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt.go
File ipv4/sockopt.go (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt.go#newcode27
ipv4/sockopt.go:27: ssoTypeInt
Where is the code that maps from these value types to the function to call?  Why
is it not in this file?

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_unix.go
File ipv4/sockopt_unix.go (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_unix.go#ne...
ipv4/sockopt_unix.go:18: getType int // option value type, must be equal or
greater than 1
Why would getType and setType ever be different?

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_windows.go
File ipv4/sockopt_windows.go (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_windows.go...
ipv4/sockopt_windows.go:15: type sockOpt struct {
Why is sockOpt not defined in sockopt.go?

https://codereview.appspot.com/140640045/diff/2070001/ipv4/thunk_compat_linux...
File ipv4/thunk_compat_linux_386.s (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/thunk_compat_linux...
ipv4/thunk_compat_linux_386.s:9: TEXT	·socketcall(SB),4,$0-36
Just use this version all the time.  It's not worth having a separate go1.4
version just to use textflag.h.

[mikio, 2014-10-01 02:59:37]

ptal

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt.go
File ipv4/sockopt.go (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt.go#newcode27
ipv4/sockopt.go:27: ssoTypeInt
ah, overlooked, thx. moved here.

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_unix.go
File ipv4/sockopt_unix.go (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_unix.go#ne...
ipv4/sockopt_unix.go:18: getType int // option value type, must be equal or
greater than 1
On 2014/09/30 14:28:04, iant wrote:
> Why would getType and setType ever be different?

Done.

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_windows.go
File ipv4/sockopt_windows.go (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/sockopt_windows.go...
ipv4/sockopt_windows.go:15: type sockOpt struct {
On 2014/09/30 14:28:04, iant wrote:
> Why is sockOpt not defined in sockopt.go?

Done.

https://codereview.appspot.com/140640045/diff/2070001/ipv4/thunk_compat_linux...
File ipv4/thunk_compat_linux_386.s (right):

https://codereview.appspot.com/140640045/diff/2070001/ipv4/thunk_compat_linux...
ipv4/thunk_compat_linux_386.s:9: TEXT	·socketcall(SB),4,$0-36
On 2014/09/30 14:28:05, iant wrote:
> Just use this version all the time.  It's not worth having a separate go1.4
> version just to use textflag.h.

Done.

[iant, 2014-10-03 00:49:51]

This CL needs to get smaller somehow.  Can you separate out the control message
stuff?  It seems somewhat independent.

[mikio, 2014-10-03 01:24:47]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-10-03 15:48:38]

LGTM

Thanks.

[mikio, 2014-10-03 23:12:49]

*** Submitted as
https://code.google.com/p/go/source/detail?r=2084e7244573&repo=net ***

go.net/ipv4: restructure sticky socket option handling

This CL chops existing sticky socket option handlers and puts them
into platform dependent sticky socket option binding table for
supporting multicast features such as source filtering for any-source
multicast, source-specific multicast.

Also adds tiny syscall shims to help to support solaris, to improve
existing platform support.

LGTM=iant
R=iant
CC=golang-codereviews
https://codereview.appspot.com/140640045