code review 97800043: go.net/ipv4: add support for dragonfly

go.net/ipv4: add support for dragonfly, enable IP_PKTINFO on darwin

This CL adds support for dragonfly and IP_PKTINFO support for darwin
with less dependency on syscall package.

Update issue 7175
Fixes issue 7172.

[mikio, 2014-04-29 01:47:36]

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

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

[gobot, 2014-05-01 21:14:39]

R=iant@golang.org (assigned by mikioh.mikioh@gmail.com)

[mikio, 2014-05-05 17:12:16]

ping

[mikio, 2014-05-09 23:44:18]

ping

do you have a spare time to have a look at this cl?
almost all the dumb stuff in this cl is just for adaptation to go1.[1-3] on all
supported platforms except go1.3+solaris and go1.[1-3]+plan9.

[iant, 2014-05-10 00:02:55]

Sorry, this fell out of my e-mail for some reason.

I don't really see the argument for using your own system call code here and not
using the syscall package.  It seems like increased maintenance but I don't see
the advantage.  I gather that you are trying to provide the same API for Go 1.1
and Go 1.3.  I recommend instead ensuring that Go 1.1 works, but for features
that require Go 1.3, only provide them when using Go 1.3.

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_bsd.go
File ipv4/sys_bsd.go (right):

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_bsd.go#newcode10
ipv4/sys_bsd.go:10: // See /usr/include/netinet/in.h.
Why not use the values from the syscall package?

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_dragonfly.go
File ipv4/sys_dragonfly.go (right):

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_dragonfly.go#newc...
ipv4/sys_dragonfly.go:5: // +build dragonfly
You don't need the +build line, given the file name.  But again, why not use the
syscall package?

https://codereview.appspot.com/97800043/diff/80001/ipv4/syscall_linux_386.go
File ipv4/syscall_linux_386.go (right):

https://codereview.appspot.com/97800043/diff/80001/ipv4/syscall_linux_386.go#...
ipv4/syscall_linux_386.go:30: func getsockopt(fd int, level int, name int, v
unsafe.Pointer, l *sysSockoptLen) error {
Using a getsockopt function instead of something like the existing GetsockoptInt
seems like a big step backward in safety.  Why do it?

[mikio, 2014-05-10 00:35:13]

On Sat, May 10, 2014 at 9:02 AM,  <iant@golang.org> wrote:

> I don't really see the argument for using your own system call code here
> and not using the syscall package.  It seems like increased maintenance
> but I don't see the advantage.  I gather that you are trying to provide
> the same API for Go 1.1 and Go 1.3. I recommend instead ensuring that
> Go 1.1 works, but for features that require Go 1.3, only provide them
> when using Go 1.3.

i thought the same first, then saw golang.org/issue/6548 and changed
my mind. keeping own syscall stuff in go.net/ipv6 works well for now,
also it makes possible to add kernel/version specific
features/modifications with no dependency on any version of go syscall
package. i think that--syscall free--is the advantage.

[iant, 2014-05-10 00:43:44]

On Fri, May 9, 2014 at 5:35 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Sat, May 10, 2014 at 9:02 AM,  <iant@golang.org> wrote:
>
>> I don't really see the argument for using your own system call code here
>> and not using the syscall package.  It seems like increased maintenance
>> but I don't see the advantage.  I gather that you are trying to provide
>> the same API for Go 1.1 and Go 1.3. I recommend instead ensuring that
>> Go 1.1 works, but for features that require Go 1.3, only provide them
>> when using Go 1.3.
>
> i thought the same first, then saw golang.org/issue/6548 and changed
> my mind. keeping own syscall stuff in go.net/ipv6 works well for now,
> also it makes possible to add kernel/version specific
> features/modifications with no dependency on any version of go syscall
> package. i think that--syscall free--is the advantage.

I think that issue 6548 just means that code that uses
syscall.ICMPv6Filter and friends needs to go into a separate file with
an appropriate build tag.

I don't yet see the argument for avoiding the syscall package.

Ian

[mikio, 2014-05-10 02:10:38]

On Sat, May 10, 2014 at 9:43 AM, Ian Lance Taylor <iant@golang.org> wrote:

> I think that issue 6548 just means that code that uses
> syscall.ICMPv6Filter and friends needs to go into a separate file with
> an appropriate build tag.

ah, you are saying "it doesn't compile" doesn't mean "it works", hm
you're right.

> I don't yet see the argument for avoiding the syscall package.

okay, let me have a try here. i'd like to reduce the use of syscall
package stuff in go.net/ipv4 the same as go/net/ipv6 for the following
reasons:

- support for multiple go versions on multiple platforms: the syscall
package contains platform-dependent stuff but has not good support for
multiple os versions. for example, go1.3 for freebsd contains a few
syscall constants that incompatible with freebsd10. it would be a pain
when we support the go-version x platform{os-version x arch} matrix.
also go never rejects any request to add new platform support.

- ability of code maintenance: by way of example, lines of code of
go.net/ipv6 has no big difference, before:after=4410:4557. also in my
experience, scraping syscall package of multiple go versions is a bit
tough than putting required constants into own package even though
both require to understand the gap between kernel implementations
(yup, this is the most heavy weight bit).

- extensibility: it's nice we are able to add new features to
go.net/ipv4 without worrying about the adaptation to syscall package
under the promise of go1 compatibility.

[mikio, 2014-05-10 02:14:09]

On Sat, May 10, 2014 at 11:10 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> ah, you are saying "it doesn't compile" doesn't mean "it works", hm
> you're right.

"it does compile" doesn't mean...

[mikio, 2014-05-10 23:52:58]

On Sat, May 10, 2014 at 9:02 AM,  <iant@golang.org> wrote:

>
https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_dragonfly.go#newc...
> ipv4/sys_dragonfly.go:5: // +build dragonfly
> You don't need the +build line, given the file name.

nope, go1 (go1.0.x) requires this tag.

[mikio, 2014-05-14 02:49:12]

*** Abandoned ***

[mikio, 2014-05-14 03:12:37]

On Wed, May 14, 2014 at 11:49 AM,  <mikioh.mikioh@gmail.com> wrote:

> *** Abandoned ***

nope, hands slipped.

[mikio, 2014-05-14 13:09:00]

PTAL

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_bsd.go
File ipv4/sys_bsd.go (right):

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_bsd.go#newcode10
ipv4/sys_bsd.go:10: // See /usr/include/netinet/in.h.
restored as much as possible.

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_dragonfly.go
File ipv4/sys_dragonfly.go (right):

https://codereview.appspot.com/97800043/diff/80001/ipv4/sys_dragonfly.go#newc...
ipv4/sys_dragonfly.go:5: // +build dragonfly
go/build of go1.1 requires this, but i'll remove this and take another way.

https://codereview.appspot.com/97800043/diff/80001/ipv4/syscall_linux_386.go
File ipv4/syscall_linux_386.go (right):

https://codereview.appspot.com/97800043/diff/80001/ipv4/syscall_linux_386.go#...
ipv4/syscall_linux_386.go:30: func getsockopt(fd int, level int, name int, v
unsafe.Pointer, l *sysSockoptLen) error {
i have a plan that improves multicast control and it will require this.

[iant, 2014-05-14 18:04:57]

I'm not comfortable signing off on code that duplicates the syscall package,
especially things like the wacky handling of socket calls on i386 GNU/Linux.  I
don't yet see any reason why it is necessary.

I think that if somebody builds go.net with an older version of Go, it's fine if
they get only a subset of the functionality.  We want go.net to remain backward
compatible with itself, and we want it to work with older versions of Go.  We
don't have to provide the same functionality for all versions of Go.

[mikio, 2014-05-15 05:14:13]

PTAL

I dropped syscall duplicates a bit.

On Thu, May 15, 2014 at 3:04 AM,  <iant@golang.org> wrote:

> I'm not comfortable signing off on code that duplicates the syscall
> package, especially things like the wacky handling of socket calls on
> i386 GNU/Linux.  I don't yet see any reason why it is necessary.

It's a long story. IIRC, first Russ stated that we never introduce
ioctl-ish syscall.Sockopt and Getsockopt, but Windows port supported
both for some reason. After a while, syscall started bloating, then
someone said "please don't add anything that is unrelated to standard
library". So that's why go.net/ipv4 and ipv6 now try to keep syscall
duplicate fragments.

Well, If you have a spare time in go1.4 cycle, I'll send a few CLs
that smoothen the surface of IP-level socket options in syscall, and
drop the remaining syscall duplicates in go.net/ipv6.

> I think that if somebody builds go.net with an older version of Go, it's
> fine if they get only a subset of the functionality.  We want go.net to
> remain backward compatible with itself, and we want it to work with
> older versions of Go.  We don't have to provide the same functionality
> for all versions of Go.

Sure, but I need it a little longer. Once go1.3 is out I'll revisit my
go1.1+go.net/ipv4 stuff.

[iant, 2014-05-15 23:31:13]

LGTM

Thanks for adjusting it.

[mikio, 2014-05-19 03:20:16]

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

go.net/ipv4: add support for dragonfly, enable IP_PKTINFO on darwin

This CL adds support for dragonfly and IP_PKTINFO support for darwin
with less dependency on syscall package.

Update issue 7175
Fixes issue 7172.

LGTM=iant
R=golang-codereviews, gobot, iant
CC=golang-codereviews
https://codereview.appspot.com/97800043