code review 9353045: go.net/ipv4: drop DIffServ and ECN constants, add ICMPType

go.net/ipv4: drop DIffServ and ECN constants, add ICMPType

This CL removes DiffServ and ECN constants assigned by IANA because
for now we don't have an appropriate package to put those constants
as part of API. There were used for the type-of-service field of IPv4
header and the traffic class field of IPv6 header.

Also adds ICMPType for convenience, makes use of internal IANA
protocol number constants instead of syscall's to prevent churning of
package syscall in the near future.

[mikio, 2013-05-12 09:19:08]

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

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

[mikio, 2013-06-04 09:58:02]

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

Please take another look.

[mikio, 2013-06-05 11:09:08]

ping

[dave_cheney.net, 2013-06-05 11:22:26]

On 2013/06/05 11:09:08, mikio wrote:
> ping

I'm sorry this got dropped. I will make some time to review it tomorrow. The
rational in the description looks sound, I just need to read all the changes.

[dave_cheney.net, 2013-06-06 11:32:22]

Please consider this wording for the description

This CL removes DiffServ and ENC constants assigned by IANA because 
for now we don't have an appropriate package to put those constants as part of
API.
These were used for the type-of-service field of IPv4 header and the traffic
class
field of IPv6 header.

Also adds ICMPType for convenience, makes use of internal IANA
protocol number constants instead of syscall's to prevent churning of
package syscall in the near future.

https://codereview.appspot.com/9353045/diff/32001/ipv4/example_test.go
File ipv4/example_test.go (right):

https://codereview.appspot.com/9353045/diff/32001/ipv4/example_test.go#newcod...
ipv4/example_test.go:153: c, err := net.ListenPacket("ip4:89", "0.0.0.0") //
OSFP for IPv4
maybe net.IPv4Zero ?

https://codereview.appspot.com/9353045/diff/32001/ipv4/example_test.go#newcod...
ipv4/example_test.go:238: c, err := net.ListenPacket("ip4:89", "0.0.0.0") //
OSPF for IPv4
same

https://codereview.appspot.com/9353045/diff/32001/ipv4/icmp.go
File ipv4/icmp.go (right):

https://codereview.appspot.com/9353045/diff/32001/ipv4/icmp.go#newcode7
ipv4/icmp.go:7: // An ICMPType represents a type of ICMP message.
s/An//

https://codereview.appspot.com/9353045/diff/32001/ipv4/mockicmp_test.go
File ipv4/mockicmp_test.go (right):

https://codereview.appspot.com/9353045/diff/32001/ipv4/mockicmp_test.go#newcode8
ipv4/mockicmp_test.go:8: "code.google.com/p/go.net/ipv4"
please move this below the stdlib packages and add a \n before it.

https://codereview.appspot.com/9353045/diff/32001/ipv4/sockopt_windows.go
File ipv4/sockopt_windows.go (right):

https://codereview.appspot.com/9353045/diff/32001/ipv4/sockopt_windows.go#new...
ipv4/sockopt_windows.go:20: err := syscall.Getsockopt(fd, int32(ianaProtocolIP),
int32(syscall.IP_TOS), (*byte)(unsafe.Pointer(&v)), &l)
ianaProtocolIP is a const, is the conversion needed ?

https://codereview.appspot.com/9353045/diff/32001/ipv4/sockopt_windows.go#new...
ipv4/sockopt_windows.go:29: err := syscall.Setsockopt(fd, int32(ianaProtocolIP),
int32(syscall.IP_TOS), (*byte)(unsafe.Pointer(&vv)), 4)
same, all the way down

[dave_cheney.net, 2013-06-06 11:32:37]

LGTM. I'd like to see one final pass before you submit.

[mikio, 2013-06-06 12:39:18]

Thanks for your review.

On Thu, Jun 6, 2013 at 8:32 PM,  <dave@cheney.net> wrote:

> Please consider this wording for the description

yup, thanks.

> ipv4/example_test.go:153: c, err := net.ListenPacket("ip4:89",
> "0.0.0.0") // OSFP for IPv4
> maybe net.IPv4Zero ?

nope, it's ListenPacket.

> ipv4/example_test.go:238: c, err := net.ListenPacket("ip4:89",
> "0.0.0.0") // OSPF for IPv4
> same

ditto.

> ipv4/icmp.go:7: // An ICMPType represents a type of ICMP message.
> s/An//

why? it looks consistent to me.

> ipv4/mockicmp_test.go:8: "code.google.com/p/go.net/ipv4"
> please move this below the stdlib packages and add a \n before it.

i really don't care but will do in another CL If you want.
for future reference, is it go idiomatic way?

[dave_cheney.net, 2013-06-06 12:42:27]

>> ipv4/example_test.go:153: c, err := net.ListenPacket("ip4:89",
>> "0.0.0.0") // OSFP for IPv4
>> maybe net.IPv4Zero ?
>
> nope, it's ListenPacket.

ok, cool.

>> ipv4/icmp.go:7: // An ICMPType represents a type of ICMP message.
>> s/An//
>
> why? it looks consistent to me.

If you're happy with the wording, SGTM.


>> ipv4/mockicmp_test.go:8: "code.google.com/p/go.net/ipv4"
>> please move this below the stdlib packages and add a \n before it.
>
> i really don't care but will do in another CL If you want.
> for future reference, is it go idiomatic way?

gofmt doesn't enforce it (for some reason I thought it did), this is just a
nit.

[mikio, 2013-06-06 12:56:28]

On Thu, Jun 6, 2013 at 9:42 PM, Dave Cheney <dave@cheney.net> wrote:

>>> ipv4/mockicmp_test.go:8: "code.google.com/p/go.net/ipv4"
>>> please move this below the stdlib packages and add a \n before it.
>>
>> i really don't care but will do in another CL If you want.
>> for future reference, is it go idiomatic way?
>
> gofmt doesn't enforce it (for some reason I thought it did), this is just a
nit.

thanks for explaining it. i'll leave it as is for now.

[mikio, 2013-06-06 13:19:11]

PTAL

https://codereview.appspot.com/9353045/diff/32001/ipv4/sockopt_windows.go
File ipv4/sockopt_windows.go (right):

https://codereview.appspot.com/9353045/diff/32001/ipv4/sockopt_windows.go#new...
ipv4/sockopt_windows.go:20: err := syscall.Getsockopt(fd, int32(ianaProtocolIP),
int32(syscall.IP_TOS), (*byte)(unsafe.Pointer(&v)), &l)
On 2013/06/06 11:32:22, dfc wrote:
> ianaProtocolIP is a const, is the conversion needed ?

Done.

https://codereview.appspot.com/9353045/diff/32001/ipv4/sockopt_windows.go#new...
ipv4/sockopt_windows.go:29: err := syscall.Setsockopt(fd, int32(ianaProtocolIP),
int32(syscall.IP_TOS), (*byte)(unsafe.Pointer(&vv)), 4)
On 2013/06/06 11:32:22, dfc wrote:
> same, all the way down

Done.

[dave_cheney.net, 2013-06-07 04:20:01]

LGTM. Thank you.

[mikio, 2013-06-07 06:02:30]

*** Abandoned ***

[mikio, 2013-06-07 06:06:25]

On Fri, Jun 7, 2013 at 3:02 PM,  <mikioh.mikioh@gmail.com> wrote:

> *** Abandoned ***
>
> https://codereview.appspot.com/9353045/

Nope, I just submitted.
In that process, not sure the reason, but I surely typed hg changed -d.
Sorry for the noise, TGIF.