code review 6482044: go.net/ipv4: new package

go.net/ipv4: new package

Package ipv4 implements IP-level socket options for the Internet
Protocol version 4. It also provides raw IP socket access methods
including IPv4 header manipulation.

Fixes issue 3684.
Fixes issue 3820.

This CL requires CL 6426047;
net: add read, write message methods to IPConn, UDPConn

[dave_cheney.net, 2012-08-23 10:54:44]

nit: please change the description to go.net/ipv4 as rietveld doesn't make it
obvious which repo this will be committed too.

[mikio, 2012-08-23 11:41:24]

done.

[mikio, 2012-08-28 21:55:12]

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

[gobot, 2012-09-01 14:40:48]

R=rsc (assigned by rsc)

[rsc, 2012-09-01 14:59:26]

[codereview just ate my last reply, which was quite long; this one will be
shorter]

I'll leave the code itself for Dave Cheney to review, if he's willing.

API looks reasonable. Once it proves stable and useful we might think about
making it net/ipv4 instead, but let's keep it here for now.

Please don't use .File() to get a file descriptor. That will disable
non-blocking I/O on the connection. The fd is intentionally hidden, precisely to
keep people from touching it behind package net's back. To get around that I
would suggest using reflect to dig into the structs and extract the fd integer.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode5
ipv4/header.go:5: // Package ipv4 implements IP-level socket options for the
Internet
This doc comment could use some more information, like an example. This seems
like quite a large package and I'm not at all sure how to use it.

[dave_cheney.net, 2012-09-03 11:04:47]

Thanks Mikio and rsc. I haven't finished reviewing this fully, but I know you
have been waiting for a long time so I wanted to give you feedback as soon as
possible. I still have to review the majority of the _test.go files.

In addition to rsc's comments about a bigger doc.go style introduction I have
some minor comments below.

This package is also very technical, so some example_test files might be in
order, but they can come later.

http://codereview.appspot.com/6482044/diff/25001/ipv4/control.go
File ipv4/control.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/control.go#newcode14
ipv4/control.go:14: mu     sync.Mutex
rawOpt is unexported so you can embed sync.Mutex directly, then you can skip
some of the forwarding methods.

http://codereview.appspot.com/6482044/diff/25001/ipv4/control.go#newcode19
ipv4/control.go:19: func (c *rawOpt) unlock()                   { c.mu.Unlock()
}
See above.

http://codereview.appspot.com/6482044/diff/25001/ipv4/control_bsd.go
File ipv4/control_bsd.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/control_bsd.go#newcode110
ipv4/control_bsd.go:110: return
nit: return nil might be more idiomatic.

http://codereview.appspot.com/6482044/diff/25001/ipv4/control_linux.go
File ipv4/control_linux.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/control_linux.go#newcode14
ipv4/control_linux.go:14: const pathset = FlagSrc | FlagDst | FlagInterface
could this have a comment please.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode25
ipv4/header.go:25: 
If there is a link to a relevant RFC, could you include it here please.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode43
ipv4/header.go:43: 
Does HeaderField need to be exported ?

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_plan9.go
File ipv4/helper_plan9.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_plan9.go#newcode27
ipv4/helper_plan9.go:27: }
Can this be moved a generic helper.go, or preferably ipv4.go? It doesn't look to
be specific to plan9.

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_posix.go
File ipv4/helper_posix.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_posix.go#newcode70
ipv4/helper_posix.go:70: func netAddrToIP4(a net.Addr) net.IP {
see comment in helper_plan9.go

http://codereview.appspot.com/6482044/diff/25001/ipv4/packet.go
File ipv4/packet.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/packet.go#newcode75
ipv4/packet.go:75: func (c *packetHandler) Write(h *Header, p []byte, cm
*ControlMessage) (err error) {
I think the named err return value is out of place here.

http://codereview.appspot.com/6482044/diff/25001/ipv4/sockopt_bsd.go
File ipv4/sockopt_bsd.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/sockopt_bsd.go#newcode83
ipv4/sockopt_bsd.go:83: }
Same as below.

http://codereview.appspot.com/6482044/diff/25001/ipv4/sockopt_bsd.go#newcode98
ipv4/sockopt_bsd.go:98: return nil
A version of this function exists in
http://codereview.appspot.com/6497044/diff/2002/src/pkg/net/sockoptip_bsd.go,
but this version avoids the call to fd.incref, which one is correct ?

[mikio, 2012-09-05 03:32:53]

On 2012/09/01 14:59:26, rsc wrote:

> [codereview just ate my last reply, which was quite long; this one will be
> shorter]

ah, perhaps my bad.

> I'll leave the code itself for Dave Cheney to review, if he's willing.

thanks Dave, rsc.

> API looks reasonable. Once it proves stable and useful we might think about
> making it net/ipv4 instead, but let's keep it here for now.

agreed.

> Please don't use .File() to get a file descriptor. That will disable
> non-blocking I/O on the connection. The fd is intentionally hidden, precisely
to
> keep people from touching it behind package net's back. To get around that I
> would suggest using reflect to dig into the structs and extract the fd
integer.

I missed that, a conversation btw you and fullung a bit ago. done, thx.

> This doc comment could use some more information, like an example. This seems
> like quite a large package and I'm not at all sure how to use it.

done.

[mikio, 2012-09-05 03:50:55]

Thanks Dave.

http://codereview.appspot.com/6482044/diff/25001/ipv4/control.go
File ipv4/control.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/control.go#newcode14
ipv4/control.go:14: mu     sync.Mutex
for now I'd keep them close.

http://codereview.appspot.com/6482044/diff/25001/ipv4/control_linux.go
File ipv4/control_linux.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/control_linux.go#newcode14
ipv4/control_linux.go:14: const pathset = FlagSrc | FlagDst | FlagInterface
On 2012/09/03 11:04:47, dfc wrote:
> could this have a comment please.

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode5
ipv4/header.go:5: // Package ipv4 implements IP-level socket options for the
Internet
On 2012/09/01 14:59:26, rsc wrote:
> This doc comment could use some more information, like an example. This seems
> like quite a large package and I'm not at all sure how to use it.

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode25
ipv4/header.go:25: 
On 2012/09/03 11:04:47, dfc wrote:
> If there is a link to a relevant RFC, could you include it here please.

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode43
ipv4/header.go:43: 
nope, it's just for debugging. thx.

http://codereview.appspot.com/6482044/diff/25001/ipv4/header.go#newcode43
ipv4/header.go:43: 
On 2012/09/03 11:04:47, dfc wrote:
> Does HeaderField need to be exported ?

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_plan9.go
File ipv4/helper_plan9.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_plan9.go#newcode27
ipv4/helper_plan9.go:27: }
On 2012/09/03 11:04:47, dfc wrote:
> Can this be moved a generic helper.go, or preferably ipv4.go? It doesn't look
to
> be specific to plan9.

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_posix.go
File ipv4/helper_posix.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/helper_posix.go#newcode70
ipv4/helper_posix.go:70: func netAddrToIP4(a net.Addr) net.IP {
On 2012/09/03 11:04:47, dfc wrote:
> see comment in helper_plan9.go

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/packet.go
File ipv4/packet.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/packet.go#newcode75
ipv4/packet.go:75: func (c *packetHandler) Write(h *Header, p []byte, cm
*ControlMessage) (err error) {
On 2012/09/03 11:04:47, dfc wrote:
> I think the named err return value is out of place here.

Done.

http://codereview.appspot.com/6482044/diff/25001/ipv4/sockopt_bsd.go
File ipv4/sockopt_bsd.go (right):

http://codereview.appspot.com/6482044/diff/25001/ipv4/sockopt_bsd.go#newcode98
ipv4/sockopt_bsd.go:98: return nil
My guess is that both fd.incref/decref are just for closing multiple socket
read/writers, network I/O goroutines, on the same socket gracefully.
So we probably might drop fd.incref/decref from socket option handlers
in net package.

[mikio, 2012-09-05 03:51:55]

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

Please take another look.

[dave_cheney.net, 2012-09-05 06:27:15]

This package is looking really good. I think one more pass and it should be good
to go.

There are a lot of comments included here, but they are comparatively minor.

My outstanding concern is the way we are stepping around the fd.incref/decref
controls in the net package. I know that your other CL is removing those
controls as well, so I will do some reading of the net source to check what they
are protecting.

Which platforms have you tested this on ? Do you need any help testing on
platforms you don't have access to ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_bsd.go
File ipv4/control_bsd.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_bsd.go#newcode109
ipv4/control_bsd.go:109: func marshalControlMessage(cm *ControlMessage) []byte {
is this a TODO here? Plan9 has it down as to be implemented. If there is nothing
to do here, or it isn't supported, maybe just note it here.

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_plan9.go
File ipv4/control_plan9.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_plan9.go#newcode12
ipv4/control_plan9.go:12: // TODO: Implement this
Go style is 

// TODO(mikioh) explanation

so people know to ask you if they get around to it before you.

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_windows.go
File ipv4/control_windows.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_windows.go#newc...
ipv4/control_windows.go:12: // TODO: Implement this
See control_plan9.go

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_plan9.go
File ipv4/dgramopt_plan9.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_plan9.go#newco...
ipv4/dgramopt_plan9.go:13: // TODO: Implement this
See previous files for suggested TODO style

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_posix.go
File ipv4/dgramopt_posix.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:138: //	err = p1.JoinGroup(<if #1, "en0">, <group address
#1, "224.0.0.248">)
This style is confusing to me, maybe something like this would be easier to
understand

ifi := "en0" // the first interface on this host
addr := "224.0.0.248" // the address of the multicast group to join
err := p1.JoinGroup(ifi, addr)

I would also encourage you to convert this to an testing.Example code example.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go
File ipv4/doc.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode4
ipv4/doc.go:4: 
This is excellent. Thank you.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode12
ipv4/doc.go:12: // on the DiffServ, differenciated services environment.
s/differenciated/differentiated/g

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode22
ipv4/doc.go:22: // field on the IPv4 header of each packet.
This is a bit of a mouthful, I suggest

When a single TCP connection carrying a data flow of multiple packets needs to
indicate the flow is important, ipv4.Conn is used to set the type-of-service
field on the IPv4 header for each packet.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode35
ipv4/doc.go:35: //			defer c.Close()
I think this should also go into an example. This can happen in a followup CL.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode40
ipv4/doc.go:40: //			err := ipv4.NewConn(c).SetTOS(0x28)
Is 0x28 a common value ? Does it need a constant ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode126
ipv4/doc.go:126: //		p.SetTOS(0x28)
yeah, probably needs a constant.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode146
ipv4/doc.go:146: // It is possible for the application to leave or re-join the
group
leave or rejoin a multicast group on the network interface

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go
File ipv4/endpoint.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode14
ipv4/endpoint.go:14: // It allows to set basic IP-level socket options such as
TOS and TTL.
It is used to control basic IP-level socket options such as TOS and TTL.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode32
ipv4/endpoint.go:32: // A PacketConn represents a packet network endpoint that
uses IPv4
that uses the IPv4 transport.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode33
ipv4/endpoint.go:33: // transport.  It allows to set several IP-level socket
options that
It is used to control several IP-level socket options including multicasting.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode71
ipv4/endpoint.go:71: // the endpoint.
sets the read deadline

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode80
ipv4/endpoint.go:80: // the endpoint.
sets the write deadline

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode89
ipv4/endpoint.go:89: func (c *PacketConn) Close() error {
I'm concerned about c.*.c = nil being used as a signal here. If these *Conn
types are shared across multiple goroutines there may need to be a mutex here to
ensure the nil is properly visible. Are the types in endpoint.go safe for used
by multiple goroutines ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode113
ipv4/endpoint.go:113: // datagram directly such as OSPF, GRE.
See PacketConn

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode123
ipv4/endpoint.go:123: // platforms and sometimes it might requrie routing
configuration.
s/requrie/require/g

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode124
ipv4/endpoint.go:124: func (c *RawConn) JoinGroup(ifi *net.Interface, group
net.Addr) error {
I think this method is unnecessary. c embeds the JoinGroup method from dgramOpt.
However the comment on this method is excellent, so if you decide to remove the
method, please don't loose the comment.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode149
ipv4/endpoint.go:149: // SetReadDeadline sets the read and write deadlines
associated with
sets the read deadline

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode159
ipv4/endpoint.go:159: // the endpoint.
sets the write deadline

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode175
ipv4/endpoint.go:175: c.packetHandler.c = nil
See previous comment about PacketConn.Close()

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode188
ipv4/endpoint.go:188: }
Why would r.packetHandler.ok not be true ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/genericopt_plan9.go
File ipv4/genericopt_plan9.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/genericopt_plan9.go#new...
ipv4/genericopt_plan9.go:12: // TODO: Implement this
See previous comments for suggested TODO style

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode48
ipv4/header.go:48: maxHeaderLen    = 60
Perhaps reference where this value comes from, ie RFC 1234 section N

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode55
ipv4/header.go:55: posTotalLen             = 2  // packet tolal length
s/tolal/total/g

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode88
ipv4/header.go:88: // NOTE: A binary header format
Note: XXXX is a binary header format

I think XXX is netlink, but please correct me.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode90
ipv4/header.go:90: // Byte order in the packet from the kernel:
Byte order is defined by the endianness of the host ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode91
ipv4/header.go:91: // - all fields are in network byte order on Linux,
I would reverse this so the OS comes before the description.

On Linux, all fields ...
On OpenBSD, all fields are ...
On other BSD platforms, almost all fields are ...

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode97
ipv4/header.go:97: // Field adjustment in the packet from the kernel:
ditto

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode103
ipv4/header.go:103: // Byte order in the packet to the kernel:
ditto

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode108
ipv4/header.go:108: //   traditional BSD-style platforms.
Should there be a \n here? The comment above does not reference this variable.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode136
ipv4/header.go:136: }
Does this need a check ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go
File ipv4/helper.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode4
ipv4/helper.go:4: 
nit: I would prefer this file called ipv4.go, but feel free to ignore, this is
just a personal preference.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode23
ipv4/helper.go:23: 
Why not use bytes.Equal ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode39
ipv4/helper.go:39: }
This check isn't needed. If a == nil, the type check will not match any of the
following

http://play.golang.org/p/Un1u1b-I3s

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode78
ipv4/helper.go:78: }
Is this case common ? Should it move to the top of the function?

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper_posix.go
File ipv4/helper_posix.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper_posix.go#newcode37
ipv4/helper_posix.go:37: if bytesEqual(mreq.Multiaddr[:], net.IPv4zero.To4()) {
see comment in helper.go re: bytes.Equal

http://codereview.appspot.com/6482044/diff/21002/ipv4/mocktransponder_test.go
File ipv4/mocktransponder_test.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/mocktransponder_test.go...
ipv4/mocktransponder_test.go:19: // runPyaloadTransponder trasmits IPv4 datagram
payloads to the
s/runPyaloadTransponder/runPayloadTransponder/g

s/trasmits/transmits/g

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go
File ipv4/multicast_test.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:16: func TestReadWriteMulticastUDPoIPPayload(t
*testing.T) {
toIPPayload ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:22: c, err := net.ListenPacket("udp4",
"224.0.0.0"+":"+"1024") // see RFC 4727
Can't this just be "224.0.0.0:1024" ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:33: dst, err := net.ResolveUDPAddr("udp4",
"224.0.0.254"+":"+"1024") // see RFC 4727
ditto

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicastlistener_test.go
File ipv4/multicastlistener_test.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicastlistener_test....
ipv4/multicastlistener_test.go:32: c, err := net.ListenPacket("udp4",
"0.0.0.0"+":"+"0")
see previous

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go
File ipv4/packet.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode11
ipv4/packet.go:11: 
Needs a comment

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode57
ipv4/packet.go:57: // message cm allows to specify the datagram path, outgoing
interface,
The control message cm allows the datagram path and the outgoing interface to be
specified. Currently only Linux supports this/is supported.

// is the Linux limitation in this package, or in the underlying OS ?

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode59
ipv4/packet.go:59: // nil unless the control of the outgoing datagram is
required.
cm may be nil if control of the outgoing datagram is not required.

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go
File ipv4/payload.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go#newcode11
ipv4/payload.go:11: 
Needs small comment

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go#newcode22
ipv4/payload.go:22: // src on the received datagram.
source address src of? the received datagram.

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go#newcode59
ipv4/payload.go:59: // unless the control of the outgoing datagram is required.
See packet.go:Write

[mikio, 2012-09-06 10:43:31]

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

Please take another look.

[mikio, 2012-09-06 10:43:47]

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_bsd.go
File ipv4/control_bsd.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_bsd.go#newcode109
ipv4/control_bsd.go:109: func marshalControlMessage(cm *ControlMessage) []byte {
On 2012/09/05 06:27:16, dfc wrote:
> is this a TODO here? Plan9 has it down as to be implemented. If there is
nothing
> to do here, or it isn't supported, maybe just note it here.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_plan9.go
File ipv4/control_plan9.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_plan9.go#newcode12
ipv4/control_plan9.go:12: // TODO: Implement this
On 2012/09/05 06:27:16, dfc wrote:
> Go style is 
> 
> // TODO(mikioh) explanation
> 
> so people know to ask you if they get around to it before you.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_windows.go
File ipv4/control_windows.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/control_windows.go#newc...
ipv4/control_windows.go:12: // TODO: Implement this
On 2012/09/05 06:27:16, dfc wrote:
> See control_plan9.go

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_plan9.go
File ipv4/dgramopt_plan9.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_plan9.go#newco...
ipv4/dgramopt_plan9.go:13: // TODO: Implement this
On 2012/09/05 06:27:16, dfc wrote:
> See previous files for suggested TODO style

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_posix.go
File ipv4/dgramopt_posix.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:138: //	err = p1.JoinGroup(<if #1, "en0">, <group address
#1, "224.0.0.248">)
On 2012/09/05 06:27:16, dfc wrote:
> This style is confusing to me, maybe something like this would be easier to
> understand
> 
> ifi := "en0" // the first interface on this host
> addr := "224.0.0.248" // the address of the multicast group to join
> err := p1.JoinGroup(ifi, addr)
> 
> I would also encourage you to convert this to an testing.Example code example.

revised and moved to doc.go.

http://codereview.appspot.com/6482044/diff/21002/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:138: //	err = p1.JoinGroup(<if #1, "en0">, <group address
#1, "224.0.0.248">)
On 2012/09/05 06:27:16, dfc wrote:
> This style is confusing to me, maybe something like this would be easier to
> understand
> 
> ifi := "en0" // the first interface on this host
> addr := "224.0.0.248" // the address of the multicast group to join
> err := p1.JoinGroup(ifi, addr)
> 
> I would also encourage you to convert this to an testing.Example code example.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go
File ipv4/doc.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode4
ipv4/doc.go:4: 
> This is excellent. Thank you.

Thanks for the wording correction.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode12
ipv4/doc.go:12: // on the DiffServ, differenciated services environment.
On 2012/09/05 06:27:16, dfc wrote:
> s/differenciated/differentiated/g

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode22
ipv4/doc.go:22: // field on the IPv4 header of each packet.
On 2012/09/05 06:27:16, dfc wrote:
> This is a bit of a mouthful, I suggest
> 
> When a single TCP connection carrying a data flow of multiple packets needs to
> indicate the flow is important, ipv4.Conn is used to set the type-of-service
> field on the IPv4 header for each packet.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode35
ipv4/doc.go:35: //			defer c.Close()
On 2012/09/05 06:27:16, dfc wrote:
> I think this should also go into an example. This can happen in a followup CL.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode40
ipv4/doc.go:40: //			err := ipv4.NewConn(c).SetTOS(0x28)
On 2012/09/05 06:27:16, dfc wrote:
> Is 0x28 a common value ? Does it need a constant ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode126
ipv4/doc.go:126: //		p.SetTOS(0x28)
On 2012/09/05 06:27:16, dfc wrote:
> yeah, probably needs a constant.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/doc.go#newcode146
ipv4/doc.go:146: // It is possible for the application to leave or re-join the
group
On 2012/09/05 06:27:16, dfc wrote:
> leave or rejoin a multicast group on the network interface

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go
File ipv4/endpoint.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode14
ipv4/endpoint.go:14: // It allows to set basic IP-level socket options such as
TOS and TTL.
On 2012/09/05 06:27:16, dfc wrote:
> It is used to control basic IP-level socket options such as TOS and TTL.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode32
ipv4/endpoint.go:32: // A PacketConn represents a packet network endpoint that
uses IPv4
On 2012/09/05 06:27:16, dfc wrote:
> that uses the IPv4 transport.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode33
ipv4/endpoint.go:33: // transport.  It allows to set several IP-level socket
options that
On 2012/09/05 06:27:16, dfc wrote:
> It is used to control several IP-level socket options including multicasting.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode71
ipv4/endpoint.go:71: // the endpoint.
On 2012/09/05 06:27:16, dfc wrote:
> sets the read deadline

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode80
ipv4/endpoint.go:80: // the endpoint.
On 2012/09/05 06:27:16, dfc wrote:
> sets the write deadline

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode89
ipv4/endpoint.go:89: func (c *PacketConn) Close() error {
On 2012/09/05 06:27:16, dfc wrote:
> I'm concerned about c.*.c = nil being used as a signal here. If these *Conn
> types are shared across multiple goroutines there may need to be a mutex here
to
> ensure the nil is properly visible. Are the types in endpoint.go safe for used
> by multiple goroutines ?

hm, looks not good idea to keep the state here.
will drop it.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode89
ipv4/endpoint.go:89: func (c *PacketConn) Close() error {
On 2012/09/05 06:27:16, dfc wrote:
> I'm concerned about c.*.c = nil being used as a signal here. If these *Conn
> types are shared across multiple goroutines there may need to be a mutex here
to
> ensure the nil is properly visible. Are the types in endpoint.go safe for used
> by multiple goroutines ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode113
ipv4/endpoint.go:113: // datagram directly such as OSPF, GRE.
On 2012/09/05 06:27:16, dfc wrote:
> See PacketConn

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode123
ipv4/endpoint.go:123: // platforms and sometimes it might requrie routing
configuration.
On 2012/09/05 06:27:16, dfc wrote:
> s/requrie/require/g

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode124
ipv4/endpoint.go:124: func (c *RawConn) JoinGroup(ifi *net.Interface, group
net.Addr) error {
On 2012/09/05 06:27:16, dfc wrote:
> I think this method is unnecessary. c embeds the JoinGroup method from
dgramOpt.
> However the comment on this method is excellent, so if you decide to remove
the
> method, please don't loose the comment.

it's just for godoc, to dedupe comments.
moved to doc.go.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode124
ipv4/endpoint.go:124: func (c *RawConn) JoinGroup(ifi *net.Interface, group
net.Addr) error {
On 2012/09/05 06:27:16, dfc wrote:
> I think this method is unnecessary. c embeds the JoinGroup method from
dgramOpt.
> However the comment on this method is excellent, so if you decide to remove
the
> method, please don't loose the comment.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode149
ipv4/endpoint.go:149: // SetReadDeadline sets the read and write deadlines
associated with
On 2012/09/05 06:27:16, dfc wrote:
> sets the read deadline

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode159
ipv4/endpoint.go:159: // the endpoint.
On 2012/09/05 06:27:16, dfc wrote:
> sets the write deadline

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode175
ipv4/endpoint.go:175: c.packetHandler.c = nil
On 2012/09/05 06:27:16, dfc wrote:
> See previous comment about PacketConn.Close()

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode188
ipv4/endpoint.go:188: }
On 2012/09/05 06:27:16, dfc wrote:
> Why would r.packetHandler.ok not be true ? 

user can easily pass (*net.IPConn)(nil) because IPConn
requires admin priv.  and I sometimes see non-error
check Dial/ListenPacket calls. but... hm, eventually...
unnecessary, will drop it.

http://codereview.appspot.com/6482044/diff/21002/ipv4/endpoint.go#newcode188
ipv4/endpoint.go:188: }
On 2012/09/05 06:27:16, dfc wrote:
> Why would r.packetHandler.ok not be true ? 

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/genericopt_plan9.go
File ipv4/genericopt_plan9.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/genericopt_plan9.go#new...
ipv4/genericopt_plan9.go:12: // TODO: Implement this
On 2012/09/05 06:27:16, dfc wrote:
> See previous comments for suggested TODO style

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode48
ipv4/header.go:48: maxHeaderLen    = 60
On 2012/09/05 06:27:16, dfc wrote:
> Perhaps reference where this value comes from, ie RFC 1234 section N

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode55
ipv4/header.go:55: posTotalLen             = 2  // packet tolal length
On 2012/09/05 06:27:16, dfc wrote:
> s/tolal/total/g

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode88
ipv4/header.go:88: // NOTE: A binary header format
On 2012/09/05 06:27:16, dfc wrote:
> Note: XXXX is a binary header format
> 
> I think XXX is netlink, but please correct me.

revised.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode88
ipv4/header.go:88: // NOTE: A binary header format
On 2012/09/05 06:27:16, dfc wrote:
> Note: XXXX is a binary header format
> 
> I think XXX is netlink, but please correct me.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode90
ipv4/header.go:90: // Byte order in the packet from the kernel:
On 2012/09/05 06:27:16, dfc wrote:
> Byte order is defined by the endianness of the host ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode91
ipv4/header.go:91: // - all fields are in network byte order on Linux,
On 2012/09/05 06:27:16, dfc wrote:
> I would reverse this so the OS comes before the description.
> 
> On Linux, all fields ...
> On OpenBSD, all fields are ...
> On other BSD platforms, almost all fields are ...

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode97
ipv4/header.go:97: // Field adjustment in the packet from the kernel:
On 2012/09/05 06:27:16, dfc wrote:
> ditto

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode103
ipv4/header.go:103: // Byte order in the packet to the kernel:
On 2012/09/05 06:27:16, dfc wrote:
> ditto

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode108
ipv4/header.go:108: //   traditional BSD-style platforms.
On 2012/09/05 06:27:16, dfc wrote:
> Should there be a \n here? The comment above does not reference this variable.

revised.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode108
ipv4/header.go:108: //   traditional BSD-style platforms.
On 2012/09/05 06:27:16, dfc wrote:
> Should there be a \n here? The comment above does not reference this variable.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode136
ipv4/header.go:136: }
On 2012/09/05 06:27:16, dfc wrote:
> Does this need a check ?

normally net.IP keeps an IPv4 address with IPv6 IPv4-mapped 
address format, so yes.

http://codereview.appspot.com/6482044/diff/21002/ipv4/header.go#newcode136
ipv4/header.go:136: }
On 2012/09/05 06:27:16, dfc wrote:
> Does this need a check ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go
File ipv4/helper.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode4
ipv4/helper.go:4: 
On 2012/09/05 06:27:16, dfc wrote:
> nit: I would prefer this file called ipv4.go, but feel free to ignore, this is
> just a personal preference.

sorry, this file contains nothing related to IPv4 facilities such as:
- protocol constants,
- protocol variables,
- message structures,
- finite state machines.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode4
ipv4/helper.go:4: 
On 2012/09/05 06:27:16, dfc wrote:
> nit: I would prefer this file called ipv4.go, but feel free to ignore, this is
> just a personal preference.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode23
ipv4/helper.go:23: 
On 2012/09/05 06:27:16, dfc wrote:
> Why not use bytes.Equal ? 

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode39
ipv4/helper.go:39: }
nice catch, thx.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode39
ipv4/helper.go:39: }
On 2012/09/05 06:27:16, dfc wrote:
> This check isn't needed. If a == nil, the type check will not match any of the
> following
> 
> http://play.golang.org/p/Un1u1b-I3s

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode78
ipv4/helper.go:78: }
On 2012/09/05 06:27:16, dfc wrote:
> Is this case common ? Should it move to the top of the function?

hm, will drop it.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper.go#newcode78
ipv4/helper.go:78: }
On 2012/09/05 06:27:16, dfc wrote:
> Is this case common ? Should it move to the top of the function?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper_posix.go
File ipv4/helper_posix.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/helper_posix.go#newcode37
ipv4/helper_posix.go:37: if bytesEqual(mreq.Multiaddr[:], net.IPv4zero.To4()) {
On 2012/09/05 06:27:16, dfc wrote:
> see comment in helper.go re: bytes.Equal

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/mocktransponder_test.go
File ipv4/mocktransponder_test.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/mocktransponder_test.go...
ipv4/mocktransponder_test.go:19: // runPyaloadTransponder trasmits IPv4 datagram
payloads to the
On 2012/09/05 06:27:16, dfc wrote:
> s/runPyaloadTransponder/runPayloadTransponder/g
> 
> s/trasmits/transmits/g

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go
File ipv4/multicast_test.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:16: func TestReadWriteMulticastUDPoIPPayload(t
*testing.T) {
On 2012/09/05 06:27:16, dfc wrote:
> toIPPayload ?

changed to IPPayloadUDP.

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:16: func TestReadWriteMulticastUDPoIPPayload(t
*testing.T) {
On 2012/09/05 06:27:16, dfc wrote:
> toIPPayload ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:22: c, err := net.ListenPacket("udp4",
"224.0.0.0"+":"+"1024") // see RFC 4727
On 2012/09/05 06:27:16, dfc wrote:
> Can't this just be "224.0.0.0:1024" ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:33: dst, err := net.ResolveUDPAddr("udp4",
"224.0.0.254"+":"+"1024") // see RFC 4727
On 2012/09/05 06:27:16, dfc wrote:
> ditto

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicastlistener_test.go
File ipv4/multicastlistener_test.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/multicastlistener_test....
ipv4/multicastlistener_test.go:32: c, err := net.ListenPacket("udp4",
"0.0.0.0"+":"+"0")
On 2012/09/05 06:27:16, dfc wrote:
> see previous

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go
File ipv4/packet.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode11
ipv4/packet.go:11: 
On 2012/09/05 06:27:16, dfc wrote:
> Needs a comment

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode57
ipv4/packet.go:57: // message cm allows to specify the datagram path, outgoing
interface,
On 2012/09/05 06:27:16, dfc wrote:
> The control message cm allows the datagram path and the outgoing interface to
be
> specified. Currently only Linux supports this/is supported.
> 
> // is the Linux limitation in this package, or in the underlying OS ?

of course it depends on platforms.
pls refer to ip(7) on linux, IP_PKTINFO.

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode57
ipv4/packet.go:57: // message cm allows to specify the datagram path, outgoing
interface,
On 2012/09/05 06:27:16, dfc wrote:
> The control message cm allows the datagram path and the outgoing interface to
be
> specified. Currently only Linux supports this/is supported.
> 
> // is the Linux limitation in this package, or in the underlying OS ?

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/packet.go#newcode59
ipv4/packet.go:59: // nil unless the control of the outgoing datagram is
required.
On 2012/09/05 06:27:16, dfc wrote:
> cm may be nil if control of the outgoing datagram is not required.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go
File ipv4/payload.go (right):

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go#newcode11
ipv4/payload.go:11: 
On 2012/09/05 06:27:16, dfc wrote:
> Needs small comment

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go#newcode22
ipv4/payload.go:22: // src on the received datagram.
On 2012/09/05 06:27:16, dfc wrote:
> source address src of? the received datagram.

Done.

http://codereview.appspot.com/6482044/diff/21002/ipv4/payload.go#newcode59
ipv4/payload.go:59: // unless the control of the outgoing datagram is required.
On 2012/09/05 06:27:16, dfc wrote:
> See packet.go:Write

Done.

[mikio, 2012-09-06 10:50:50]

Hi Dave,

On Wed, Sep 5, 2012 at 3:27 PM,  <dave@cheney.net> wrote:

> There are a lot of comments included here, but they are comparatively
> minor.

Thanks for your review, much appreciated.

> Which platforms have you tested this on ?

Usually I use freebsd and/or darwin snow leopard.
Sometimes linux for linux depends option testing such as IP_PKTINFO.

> Do you need any help testing
> on platforms you don't have access to ?

Sure, thanks!
I'm happy if you can run Windows 2000, XP, Vista or 7 or 8.

[dave_cheney.net, 2012-09-06 12:40:36]

LGTM modulo the minor tweaks to the comments below.

I have tested on linux/amd64 and linux/arm and all looks good.

As there are some prerequisite changes to go in first, can you please find
someone, or ask on the list, who has some windows systems to test this CL. There
is a lot of code stubbed out on windows and I don't want the tests to fail
because windows doesn't support feature X.

http://codereview.appspot.com/6482044/diff/24036/ipv4/control_linux.go
File ipv4/control_linux.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/control_linux.go#newcode14
ipv4/control_linux.go:14: // Linux provides a convenience path control option
IP_PKTINFO that
s/convenience/convenient/g

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go
File ipv4/dgramopt_posix.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:67: // should be copied, sent back to the originator.
suggest: should be copied and send back to the originator.

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:80: // should be copied, sent back to the originator.
See above

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:111: // LeaveGroup leaves the group address group on the
interface ifi.
Does there need to be a word of warning if ifi is nil ?

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go
File ipv4/doc.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode8
ipv4/doc.go:8: // The package provides IP-level socket options that allow to
that allow manipulation of IPv4 facilities.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode58
ipv4/doc.go:58: // start multicasting, at least joining network interfaces and
group
s/start/you begin/g
s/at least/at a minimum/g

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode80
ipv4/doc.go:80: // Second, the application joins groups, starts listenting to
the
s/listenting/listening/g

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode82
ipv4/doc.go:82: // the service port for transport layer protocols does not
matter with
suggest protocol not protocols as we can only do one per address, right ?

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode96
ipv4/doc.go:96: // The application might set per packet basis control message
delete basis

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode108
ipv4/doc.go:108: // interests by using the control message that contains the
are of interest

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode109
ipv4/doc.go:109: // destination addrees of the received packet.
address

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode126
ipv4/doc.go:126: // The application also could send both unicast and multicast
packets.
can also send

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode176
ipv4/doc.go:176: // UDP port to join the same group address because the net
package
full stop after address, start the new sentence a The

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode177
ipv4/doc.go:177: // provides a socket that listens to a wildcard address with
resuable
the new package will provide a socket

http://codereview.appspot.com/6482044/diff/24036/ipv4/endpoint.go
File ipv4/endpoint.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/endpoint.go#newcode50
ipv4/endpoint.go:50: // SetControlMessage sets the per packet basis IP-level
socket options.
delete basis

http://codereview.appspot.com/6482044/diff/24036/ipv4/endpoint.go#newcode97
ipv4/endpoint.go:97: // NewPacketConn returns a new PacketConn using c for
network I/O.
using c as its underlying transport ?

http://codereview.appspot.com/6482044/diff/24036/ipv4/example_test.go
File ipv4/example_test.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/example_test.go#newcode136
ipv4/example_test.go:136: AllSPFRouters        = net.IPv4(224, 0, 0, 5)
AllOSPFRouters ?

http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go#newcode48
ipv4/header.go:48: maxHeaderLen = 60 // for a while, until the day interesting
I-D/RFC reuse the VHL field comes
maybe just write // sensible default, revisit if later RFCs define new VHL (?
what is that) fields.

http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go#newcode125
ipv4/header.go:125: // incoming, outgoing IPv4 header.  On the other hand,
Linux, OpenBSD
This doesn't read very well, but I don't understand the subject matter well
enough to make a suggestion. If you are able, could you please make another pass
at rewording it. It might be useful to define the difference between new IP
input and traditional raw IP socket IO.

http://codereview.appspot.com/6482044/diff/24036/ipv4/header_test.go
File ipv4/header_test.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/header_test.go#newcode36
ipv4/header_test.go:36: }
Please add a TODO to add some non conforming test cases here. We can tackle them
in a following CL.

http://codereview.appspot.com/6482044/diff/24036/ipv4/helper_unix.go
File ipv4/helper_unix.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/helper_unix.go#newcode19
ipv4/helper_unix.go:19: return -1, errInvalidConnType
It is more traditional to return the zero value along with an err value. I
believe this is to discourage people from trying to divine meaning from the
return value without checking the error. If it is convenient I think these
should be zero.

http://codereview.appspot.com/6482044/diff/24036/ipv4/packet.go
File ipv4/packet.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/packet.go#newcode64
ipv4/packet.go:64: //	Version       = don't care
Surely this should be 4 ?

[mikio, 2012-09-06 13:12:35]

On Thu, Sep 6, 2012 at 9:40 PM,  <dave@cheney.net> wrote:

> I have tested on linux/amd64 and linux/arm and all looks good.

Thanks.

>
http://codereview.appspot.com/6482044/diff/24036/ipv4/example_test.go#newcode136
> ipv4/example_test.go:136: AllSPFRouters        = net.IPv4(224, 0, 0, 5)
> AllOSPFRouters ?

AllSPFRouters, AllDRouters from RFC 2328.
(oops AllDRouters, not AllDesignatedRouters, will change)

> http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go#newcode125
> ipv4/header.go:125: // incoming, outgoing IPv4 header.  On the other
> hand, Linux, OpenBSD
> This doesn't read very well, but I don't understand the subject matter
> well enough to make a suggestion. If you are able, could you please make
> another pass at rewording it. It might be useful to define the
> difference between new IP input and traditional raw IP socket IO.

Hm, I will drop that redundant note for ParseHeader and Marshal.

>
http://codereview.appspot.com/6482044/diff/24036/ipv4/helper_unix.go#newcode19
> ipv4/helper_unix.go:19: return -1, errInvalidConnType
> It is more traditional to return the zero value along with an err value.
> I believe this is to discourage people from trying to divine meaning
> from the return value without checking the error. If it is convenient I
> think these should be zero.

Not sure file descriptor value = 0 is good for error result, but will change.

[mikio, 2012-09-06 15:09:16]

all done, thanks for your review, Dave.

will back to the stdlib tree for preparing a move, still
remains a lot of works such as packing, carpet cleaning,
replumbing, etc.

http://codereview.appspot.com/6482044/diff/24036/ipv4/control_linux.go
File ipv4/control_linux.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/control_linux.go#newcode14
ipv4/control_linux.go:14: // Linux provides a convenience path control option
IP_PKTINFO that
On 2012/09/06 12:40:36, dfc wrote:
> s/convenience/convenient/g

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go
File ipv4/dgramopt_posix.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:67: // should be copied, sent back to the originator.
On 2012/09/06 12:40:36, dfc wrote:
> suggest: should be copied and send back to the originator.

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:80: // should be copied, sent back to the originator.
On 2012/09/06 12:40:36, dfc wrote:
> See above

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/dgramopt_posix.go#newco...
ipv4/dgramopt_posix.go:111: // LeaveGroup leaves the group address group on the
interface ifi.
On 2012/09/06 12:40:36, dfc wrote:
> Does there need to be a word of warning if ifi is nil ?

No, cannot say anymore, a lot of "undefined behavior" things 
out there. For example, on BSD variants we rely on a interface 
address to specify the multicast i/f. What happens if that address 
is disabled or removed from the i/f? certain thing is that kernel
will try to look up the IP listener with the args we passed to.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go
File ipv4/doc.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode8
ipv4/doc.go:8: // The package provides IP-level socket options that allow to
On 2012/09/06 12:40:36, dfc wrote:
> that allow manipulation of IPv4 facilities.

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode58
ipv4/doc.go:58: // start multicasting, at least joining network interfaces and
group
On 2012/09/06 12:40:36, dfc wrote:
> s/start/you begin/g
> s/at least/at a minimum/g

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode80
ipv4/doc.go:80: // Second, the application joins groups, starts listenting to
the
On 2012/09/06 12:40:36, dfc wrote:
> s/listenting/listening/g

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode82
ipv4/doc.go:82: // the service port for transport layer protocols does not
matter with
On 2012/09/06 12:40:36, dfc wrote:
> suggest protocol not protocols as we can only do one per address, right ?

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode96
ipv4/doc.go:96: // The application might set per packet basis control message
On 2012/09/06 12:40:36, dfc wrote:
> delete basis

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode108
ipv4/doc.go:108: // interests by using the control message that contains the
On 2012/09/06 12:40:36, dfc wrote:
> are of interest

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode109
ipv4/doc.go:109: // destination addrees of the received packet.
On 2012/09/06 12:40:36, dfc wrote:
> address

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode126
ipv4/doc.go:126: // The application also could send both unicast and multicast
packets.
On 2012/09/06 12:40:36, dfc wrote:
> can also send

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode176
ipv4/doc.go:176: // UDP port to join the same group address because the net
package
On 2012/09/06 12:40:36, dfc wrote:
> full stop after address, start the new sentence a The

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/doc.go#newcode177
ipv4/doc.go:177: // provides a socket that listens to a wildcard address with
resuable
On 2012/09/06 12:40:36, dfc wrote:
> the new package will provide a socket

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/endpoint.go
File ipv4/endpoint.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/endpoint.go#newcode50
ipv4/endpoint.go:50: // SetControlMessage sets the per packet basis IP-level
socket options.
On 2012/09/06 12:40:36, dfc wrote:
> delete basis

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/endpoint.go#newcode97
ipv4/endpoint.go:97: // NewPacketConn returns a new PacketConn using c for
network I/O.
On 2012/09/06 12:40:36, dfc wrote:
> using c as its underlying transport ?

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/example_test.go
File ipv4/example_test.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/example_test.go#newcode136
ipv4/example_test.go:136: AllSPFRouters        = net.IPv4(224, 0, 0, 5)
On 2012/09/06 12:40:36, dfc wrote:
> AllOSPFRouters ?

I'd prefer the term in RFC 2328 because this is just an example.

http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go
File ipv4/header.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go#newcode48
ipv4/header.go:48: maxHeaderLen = 60 // for a while, until the day interesting
I-D/RFC reuse the VHL field comes
On 2012/09/06 12:40:36, dfc wrote:
> maybe just write // sensible default, revisit if later RFCs define new VHL (?
> what is that) fields.

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/header.go#newcode125
ipv4/header.go:125: // incoming, outgoing IPv4 header.  On the other hand,
Linux, OpenBSD
On 2012/09/06 12:40:36, dfc wrote:
> This doesn't read very well, but I don't understand the subject matter well
> enough to make a suggestion. If you are able, could you please make another
pass
> at rewording it. It might be useful to define the difference between new IP
> input and traditional raw IP socket IO.

will drop it.

http://codereview.appspot.com/6482044/diff/24036/ipv4/header_test.go
File ipv4/header_test.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/header_test.go#newcode36
ipv4/header_test.go:36: }
On 2012/09/06 12:40:36, dfc wrote:
> Please add a TODO to add some non conforming test cases here. We can tackle
them
> in a following CL.

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/helper_unix.go
File ipv4/helper_unix.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/helper_unix.go#newcode19
ipv4/helper_unix.go:19: return -1, errInvalidConnType
On 2012/09/06 12:40:36, dfc wrote:
> It is more traditional to return the zero value along with an err value. I
> believe this is to discourage people from trying to divine meaning from the
> return value without checking the error. If it is convenient I think these
> should be zero.

Done.

http://codereview.appspot.com/6482044/diff/24036/ipv4/packet.go
File ipv4/packet.go (right):

http://codereview.appspot.com/6482044/diff/24036/ipv4/packet.go#newcode64
ipv4/packet.go:64: //	Version       = don't care
On 2012/09/06 12:40:36, dfc wrote:
> Surely this should be 4 ?

Done.

[brainman, 2012-09-07 02:04:40]

It fails to build for windows:

# GOOS=windows go test -c
# code.google.com/p/go.net/ipv4
./packet.go:28: c.c.ReadMsgIP undefined (type *net.IPConn has no field or method
ReadMsgIP)
./packet.go:95: c.c.WriteMsgIP undefined (type *net.IPConn has no field or
method WriteMsgIP)
./payload.go:32: rd.ReadMsgUDP undefined (type *net.UDPConn has no field or
method ReadMsgUDP)
./payload.go:37: rd.ReadMsgIP undefined (type *net.IPConn has no field or method
ReadMsgIP)
./payload.go:71: wr.WriteMsgUDP undefined (type *net.UDPConn has no field or
method WriteMsgUDP)
./payload.go:73: wr.WriteMsgIP undefined (type *net.IPConn has no field or
method WriteMsgIP)
#

You could use http://code.google.com/p/go-wiki/wiki/WindowsCrossCompiling to see
it yourself.

Alex

[mikio, 2012-09-07 03:28:11]

On Fri, Sep 7, 2012 at 11:04 AM,  <alex.brainman@gmail.com> wrote:

> It fails to build for windows:

thanks for your confirmation.
as in CL description  this CL requires belows:
http://codereview.appspot.com/6426047/
http://codereview.appspot.com/6506063/

[brainman, 2012-09-07 03:53:42]

On 2012/09/07 03:28:11, mikio wrote:
> On Fri, Sep 7, 2012 at 11:04 AM,  <mailto:alex.brainman@gmail.com> wrote:
> 
> > It fails to build for windows:
> 

My bad, it builds and runs fine:

C:\>test -test.v
=== RUN TestMarshalHeader
--- PASS: TestMarshalHeader (0.00 seconds)
=== RUN TestParseHeader
--- PASS: TestParseHeader (0.00 seconds)
PASS

[mikio, 2012-09-07 08:01:04]

Hello rsc@golang.org, dave@cheney.net, alex.brainman@gmail.com (cc:
gobot@golang.org, golang-dev@googlegroups.com),

Please take another look.

[mikio, 2012-09-07 08:03:08]

On Fri, Sep 7, 2012 at 5:01 PM,  <mikioh.mikioh@gmail.com> wrote:

> Please take another look.

replumbing to windows done.

[brainman, 2012-09-10 04:14:32]

On 2012/09/07 08:03:08, mikio wrote:
> 
> replumbing to windows done.

C:\go\path\src\code.google.com\p\go.net\ipv4>go test -v
=== RUN TestMarshalHeader
--- PASS: TestMarshalHeader (0.00 seconds)
=== RUN TestParseHeader
--- PASS: TestParseHeader (0.00 seconds)
=== RUN TestUDPMulticastSockopt
--- PASS: TestUDPMulticastSockopt (0.00 seconds)
=== RUN TestIPMulticastSockopt
--- PASS: TestIPMulticastSockopt (0.00 seconds)
multicastsockopt_test.go:63:    skipping test; must be root
=== RUN TestTCPUnicastSockopt
--- FAIL: TestTCPUnicastSockopt (0.00 seconds)
unicastsockopt_test.go:115:     ipv4.Conn.SetTOS failed: setsockopt: An invalid
argument was supplied.
=== RUN TestUDPUnicastSockopt
--- PASS: TestUDPUnicastSockopt (0.00 seconds)
=== RUN TestIPUnicastSockopt
--- PASS: TestIPUnicastSockopt (0.00 seconds)
unicastsockopt_test.go:90:      skipping test; must be root
FAIL
exit status 1
FAIL    code.google.com/p/go.net/ipv4   0.094s

Alex

[mikio, 2012-09-10 05:56:24]

On 2012/09/10 04:14:32, brainman wrote:

> === RUN TestTCPUnicastSockopt
> --- FAIL: TestTCPUnicastSockopt (0.00 seconds)
> unicastsockopt_test.go:115:     ipv4.Conn.SetTOS failed: setsockopt: An
invalid
> argument was supplied.

Urgh, sorry and thanks for the test run.
IP_TOS option is supported on 8 and Server 2012 only.
Will drop it in the case of windows.

[mikio, 2012-09-24 22:00:33]

Hello rsc@golang.org, dave@cheney.net, alex.brainman@gmail.com (cc:
gobot@golang.org, golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-09-25 13:10:32]

Hello,

Here are some final tweaks. I think this CL as it stands now represents an
excellent start to this package and I don't think additional review cycles would
generate significant value. 

If you are happy with it, and you have all the other dependent CL's committed,
then I would be happy to see this submitted as it stands now so that people can
start to use it.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go
File ipv4/doc.go (right):

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode8
ipv4/doc.go:8: // The package provides IP-level socket options that allow
manipulaton
s/manipulaton/manipulation/

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode12
ipv4/doc.go:12: // field on the DiffServ, differentiated services environment.
maybe s/on the/in a/, but i'm not sure, i've never used such a thing.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode82
ipv4/doc.go:82: // this operation because joining groups affects only network
and link
s/because/as

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode152
ipv4/doc.go:152: // 1024 might join two diffrent groups across over two
different
s/diffrent/different/

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode174
ipv4/doc.go:174: // It is possible for multiple UDP listeners that listen to the
same
s/listen to/listen on/ -- maybe ?

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode176
ipv4/doc.go:176: // provide a socket that listens to a wildcard address with
resuable
s/resuable/reusable/

[brainman, 2012-09-26 01:36:00]

LGTM

All tests PASS on windows. Thank you.

Alex

[mikio, 2012-09-26 12:00:53]

On Tue, Sep 25, 2012 at 10:10 PM,  <dave@cheney.net> wrote:

> Here are some final tweaks.

I much appreciate your comments and suggestions. Thanks.

[mikio, 2012-09-26 12:02:05]

On Wed, Sep 26, 2012 at 10:36 AM,  <alex.brainman@gmail.com> wrote:

> All tests PASS on windows. Thank you.

Thanks for your confirmation.

[mikio, 2012-09-26 12:03:23]

*** Submitted as
http://code.google.com/p/go/source/detail?r=7336440da745&repo=net ***

go.net/ipv4: new package

Package ipv4 implements IP-level socket options for the Internet
Protocol version 4. It also provides raw IP socket access methods
including IPv4 header manipulation.

Fixes issue 3684.
Fixes issue 3820.

This CL requires CL 6426047;
net: add read, write message methods to IPConn, UDPConn

R=rsc, dave, alex.brainman
CC=gobot, golang-dev
http://codereview.appspot.com/6482044

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go
File ipv4/doc.go (right):

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode8
ipv4/doc.go:8: // The package provides IP-level socket options that allow
manipulaton
On 2012/09/25 13:10:33, dfc wrote:
> s/manipulaton/manipulation/

Done.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode12
ipv4/doc.go:12: // field on the DiffServ, differentiated services environment.
On 2012/09/25 13:10:33, dfc wrote:
> maybe s/on the/in a/, but i'm not sure, i've never used such a thing.

will take "in a", thx.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode12
ipv4/doc.go:12: // field on the DiffServ, differentiated services environment.
On 2012/09/25 13:10:33, dfc wrote:
> maybe s/on the/in a/, but i'm not sure, i've never used such a thing.

Done.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode82
ipv4/doc.go:82: // this operation because joining groups affects only network
and link
On 2012/09/25 13:10:33, dfc wrote:
> s/because/as

Done.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode152
ipv4/doc.go:152: // 1024 might join two diffrent groups across over two
different
On 2012/09/25 13:10:33, dfc wrote:
> s/diffrent/different/

Done.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode174
ipv4/doc.go:174: // It is possible for multiple UDP listeners that listen to the
same
On 2012/09/25 13:10:33, dfc wrote:
> s/listen to/listen on/ -- maybe ?

Done.

http://codereview.appspot.com/6482044/diff/41001/ipv4/doc.go#newcode176
ipv4/doc.go:176: // provide a socket that listens to a wildcard address with
resuable
On 2012/09/25 13:10:33, dfc wrote:
> s/resuable/reusable/

Done.