code review 9843044: go.net/ipv6: new package

go.net/ipv6: new package

Package ipv6 implements IP-level socket options for the Internet
Protocol version 6. It also provides datagram based network I/O
methods specific to the IPv6 and higher layer protocols.

Fixes issue 5538.

[mikio, 2013-05-30 23:44:13]

Hello dave@cheney.net (cc: golang-dev@googlegroups.com),

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

[mikio, 2013-05-30 23:50:34]

> Description:
> go.net/ipv6: new package

As per discussion
https://groups.google.com/d/msg/golang-dev/zbMTkiAq-Hw/N2Q6zZ4EjRYJ,
we've decided to keep IP-level options out of stdlib, and removed
almost all IP-level options for both IPv4 and IPv6 from stdlib (except
LinstenMulticastUDP). The options for IPv4 already exists at
go.net/ipv4, so this CL fills a gap for IPv6.

[dave_cheney.net, 2013-06-01 05:32:07]

Hello,

I think this CL is pretty close, and I don't want to spend too long polishing it
as I have a feeling we'll be doing a few followups.

Please take a look at these comments. 

I'd like to hear from the windows and *bsd's to head off any breakage on their
builders.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go
File ipv6/control.go (right):

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode15
ipv6/control.go:15: errNotSupported    = errors.New("not supported")
this is a policy question, i've noticed that some packages are adding a prefix
to their error messages, ie, "ipv6: not supported"

I am not sure if this is appropriate here.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode42
ipv6/control.go:42: func (c *rawOpt) set(f ControlFlags)        { c.cflags |= f
}
could you please add a small unit test for these flags, esp as they are not
holding the mutex when called.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode46
ipv6/control.go:46: type ControlFlags uint
This public symbol needs a comment.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode58
ipv6/control.go:58: // packet basis IP-level socket option data.
// A ControlMessage represents per package based IP-level socket control
options.

^ maybe.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode67
ipv6/control.go:67: //
Can these two be uint8s rather than ints ?

https://codereview.appspot.com/9843044/diff/7049/ipv6/endpoint.go
File ipv6/endpoint.go (right):

https://codereview.appspot.com/9843044/diff/7049/ipv6/endpoint.go#newcode26
ipv6/endpoint.go:26: // with the endpoint.
a path MTU value, or the path MTU value ?

https://codereview.appspot.com/9843044/diff/7049/ipv6/helper.go
File ipv6/helper.go (right):

https://codereview.appspot.com/9843044/diff/7049/ipv6/helper.go#newcode25
ipv6/helper.go:25: }
is it worth handling the default case here ? Is that an error ?

[mikio, 2013-06-04 03:46:08]

PTAL

Thanks for your review, as usual.

> I'd like to hear from the windows and *bsd's to head off any breakage on their
builders.

Cross compilations passed a few days ago and almost all tests are disabled on
Windows. BSD bros are always fine.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go
File ipv6/control.go (right):

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode15
ipv6/control.go:15: errNotSupported    = errors.New("not supported")
I'd like to keep as it is until when we fix issue 4856.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode42
ipv6/control.go:42: func (c *rawOpt) set(f ControlFlags)        { c.cflags |= f
}
On 2013/06/01 05:32:07, dfc wrote:
> could you please add a small unit test for these flags, esp as they are not
> holding the mutex when called.

Done.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode46
ipv6/control.go:46: type ControlFlags uint
On 2013/06/01 05:32:07, dfc wrote:
> This public symbol needs a comment.

Done.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode58
ipv6/control.go:58: // packet basis IP-level socket option data.
Changed; also the comment on SetControlMessage.

https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode67
ipv6/control.go:67: //
We use those for both sticky and per-packet options. So let's leave it as it is.

https://codereview.appspot.com/9843044/diff/7049/ipv6/endpoint.go
File ipv6/endpoint.go (right):

https://codereview.appspot.com/9843044/diff/7049/ipv6/endpoint.go#newcode26
ipv6/endpoint.go:26: // with the endpoint.
Hm, probably "a path MTU" because it's one of many; uh-oh path MTU/packetization
layer path MTU over multiple paths consist of multiple hops.

https://codereview.appspot.com/9843044/diff/7049/ipv6/helper.go
File ipv6/helper.go (right):

https://codereview.appspot.com/9843044/diff/7049/ipv6/helper.go#newcode25
ipv6/helper.go:25: }
It's fine as is; see ReadFrom in payload_cmsg.go.

[dave_cheney.net, 2013-06-04 03:56:25]

LGTM. Obviously this is the first of several CLs, so lets get it in early in the
1.2 cycle and iterate from there. 

Also, lol @ BSD bros. 



On 04/06/2013, at 13:46, mikioh.mikioh@gmail.com wrote:

> PTAL
> 
> Thanks for your review, as usual.
> 
>> I'd like to hear from the windows and *bsd's to head off any breakage
> on their builders.
> 
> Cross compilations passed a few days ago and almost all tests are
> disabled on Windows. BSD bros are always fine.
> 
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go
> File ipv6/control.go (right):
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode15
> ipv6/control.go:15: errNotSupported    = errors.New("not supported")
> I'd like to keep as it is until when we fix issue 4856.
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode42
> ipv6/control.go:42: func (c *rawOpt) set(f ControlFlags)        {
> c.cflags |= f }
> On 2013/06/01 05:32:07, dfc wrote:
>> could you please add a small unit test for these flags, esp as they
> are not
>> holding the mutex when called.
> 
> Done.
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode46
> ipv6/control.go:46: type ControlFlags uint
> On 2013/06/01 05:32:07, dfc wrote:
>> This public symbol needs a comment.
> 
> Done.
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode58
> ipv6/control.go:58: // packet basis IP-level socket option data.
> Changed; also the comment on SetControlMessage.
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/control.go#newcode67
> ipv6/control.go:67: //
> We use those for both sticky and per-packet options. So let's leave it
> as it is.
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/endpoint.go
> File ipv6/endpoint.go (right):
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/endpoint.go#newcode26
> ipv6/endpoint.go:26: // with the endpoint.
> Hm, probably "a path MTU" because it's one of many; uh-oh path
> MTU/packetization layer path MTU over multiple paths consist of multiple
> hops.
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/helper.go
> File ipv6/helper.go (right):
> 
> https://codereview.appspot.com/9843044/diff/7049/ipv6/helper.go#newcode25
> ipv6/helper.go:25: }
> It's fine as is; see ReadFrom in payload_cmsg.go.
> 
> https://codereview.appspot.com/9843044/

[mikio, 2013-06-04 08:43:07]

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

go.net/ipv6: new package

Package ipv6 implements IP-level socket options for the Internet
Protocol version 6. It also provides datagram based network I/O
methods specific to the IPv6 and higher layer protocols.

Fixes issue 5538.

R=dave
CC=golang-dev
https://codereview.appspot.com/9843044