code review 15020047: go.net/ipv6: add ICMP checksum test

go.net/ipv6: add ICMP checksum test

This CL enables testing raw ICMP packets with and without a setting of
ICMP checksum field.

Actually it's just for Linux because almost all BSD variants never
allow us to touch the checksum field of ICMP but Linux does when we use
the value 255 for the socket level.

[mikio, 2013-10-21 10:03:12]

Hello golang-dev@googlegroups.com, 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-11-02 02:52:17]

ping

[dave_cheney.net, 2013-11-02 03:34:13]

Some tiny nits. I don't claim to understand exactly what this test is testing,
but I think I have the general idea. I'll try to run this on the machines I have
available to me and see if anything blows up.

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go
File ipv6/mockicmp_test.go (right):

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:14: b := make([]byte, 2*net.IPv6len+8)
is 8 a magic constant ? could you make a const here to capture it.

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:15: copy(b[:net.IPv6len], src)
this is a big magical. is it worth a comment why you copy src then dst when
their positions in the header are reversed.

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:40: b = append(psh, b...)
nice

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:53: b[off], b[off+1], b[off+2], b[off+3] = byte(l>>24),
byte(l>>16), byte(l>>8), byte(l)
is it worth importing encoding/binary for this ?

https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go
File ipv6/multicast_test.go (right):

https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go#ne...
ipv6/multicast_test.go:107: pshicmp :=
ipv6PseudoHeader(c.LocalAddr().(*net.IPAddr).IP, dst.IP, 58)
58 is a magic number, is it the IPv6 ether type ?

[mikio, 2013-11-02 03:40:19]

On Sat, Nov 2, 2013 at 12:34 PM,  <dave@cheney.net> wrote:

> ipv6/mockicmp_test.go:15: copy(b[:net.IPv6len], src)
> this is a big magical. is it worth a comment why you copy src then dst
> when their positions in the header are reversed.

good catch! that's a silly bug, thx.

[mikio, 2013-11-02 03:54:37]

On Sat, Nov 2, 2013 at 12:40 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

>> when their positions in the header are reversed.
>
> good catch! that's a silly bug, thx.

nope, a bit confused; they are not reversed.

[mikio, 2013-11-02 04:38:14]

ptal

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go
File ipv6/mockicmp_test.go (right):

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:14: b := make([]byte, 2*net.IPv6len+8)
On 2013/11/02 03:34:14, dfc wrote:
> is 8 a magic constant ? could you make a const here to capture it.

Done.

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:15: copy(b[:net.IPv6len], src)
On 2013/11/02 03:34:14, dfc wrote:
> this is a big magical. is it worth a comment why you copy src then dst when
> their positions in the header are reversed.

not reversed, see http://tools.ietf.org/html/rfc2460#section-8.1

https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
ipv6/mockicmp_test.go:53: b[off], b[off+1], b[off+2], b[off+3] = byte(l>>24),
byte(l>>16), byte(l>>8), byte(l)
On 2013/11/02 03:34:14, dfc wrote:
> is it worth importing encoding/binary for this ?

no other places need binary pkg so leave it as it is for now.

https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go
File ipv6/multicast_test.go (right):

https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go#ne...
ipv6/multicast_test.go:107: pshicmp :=
ipv6PseudoHeader(c.LocalAddr().(*net.IPAddr).IP, dst.IP, 58)
On 2013/11/02 03:34:14, dfc wrote:
> 58 is a magic number, is it the IPv6 ether type ?

no, that's a protocol number.

interesting, why do you think that ipv6 upper layer stuff needs ethernet types
to calculate its checksum? iirc, ietf derivarables basically implement
"lower-layer independency".

[dave_cheney.net, 2013-11-02 05:10:06]

On 2013/11/02 04:38:14, mikio wrote:
> ptal
> 
> https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go
> File ipv6/mockicmp_test.go (right):
> 
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
> ipv6/mockicmp_test.go:14: b := make([]byte, 2*net.IPv6len+8)
> On 2013/11/02 03:34:14, dfc wrote:
> > is 8 a magic constant ? could you make a const here to capture it.
> 
> Done.
> 
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
> ipv6/mockicmp_test.go:15: copy(b[:net.IPv6len], src)
> On 2013/11/02 03:34:14, dfc wrote:
> > this is a big magical. is it worth a comment why you copy src then dst when
> > their positions in the header are reversed.
> 
> not reversed, see http://tools.ietf.org/html/rfc2460#section-8.1
> 
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
> ipv6/mockicmp_test.go:53: b[off], b[off+1], b[off+2], b[off+3] = byte(l>>24),
> byte(l>>16), byte(l>>8), byte(l)
> On 2013/11/02 03:34:14, dfc wrote:
> > is it worth importing encoding/binary for this ?
> 
> no other places need binary pkg so leave it as it is for now.

SGTM.

> 
> https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go
> File ipv6/multicast_test.go (right):
> 
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go#ne...
> ipv6/multicast_test.go:107: pshicmp :=
> ipv6PseudoHeader(c.LocalAddr().(*net.IPAddr).IP, dst.IP, 58)
> On 2013/11/02 03:34:14, dfc wrote:
> > 58 is a magic number, is it the IPv6 ether type ?
> 
> no, that's a protocol number.
> 
> interesting, why do you think that ipv6 upper layer stuff needs ethernet types
> to calculate its checksum? iirc, ietf derivarables basically implement
> "lower-layer independency".

I was mistaken, I should have said protocol number, not ether type.

[dave_cheney.net, 2013-11-02 06:29:24]

LGTM. Very nice.

On Sat, Nov 2, 2013 at 4:10 PM,  <dave@cheney.net> wrote:
> On 2013/11/02 04:38:14, mikio wrote:
>>
>> ptal
>
>
>
> https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go
>>
>> File ipv6/mockicmp_test.go (right):
>
>
>
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
>>
>> ipv6/mockicmp_test.go:14: b := make([]byte, 2*net.IPv6len+8)
>> On 2013/11/02 03:34:14, dfc wrote:
>> > is 8 a magic constant ? could you make a const here to capture it.
>
>
>> Done.
>
>
>
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
>>
>> ipv6/mockicmp_test.go:15: copy(b[:net.IPv6len], src)
>> On 2013/11/02 03:34:14, dfc wrote:
>> > this is a big magical. is it worth a comment why you copy src then
>
> dst when
>>
>> > their positions in the header are reversed.
>
>
>> not reversed, see http://tools.ietf.org/html/rfc2460#section-8.1
>
>
>
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/mockicmp_test.go#new...
>>
>> ipv6/mockicmp_test.go:53: b[off], b[off+1], b[off+2], b[off+3] =
>
> byte(l>>24),
>>
>> byte(l>>16), byte(l>>8), byte(l)
>> On 2013/11/02 03:34:14, dfc wrote:
>> > is it worth importing encoding/binary for this ?
>
>
>> no other places need binary pkg so leave it as it is for now.
>
>
> SGTM.
>
>
>
>
> https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go
>>
>> File ipv6/multicast_test.go (right):
>
>
>
>
https://codereview.appspot.com/15020047/diff/210001/ipv6/multicast_test.go#ne...
>>
>> ipv6/multicast_test.go:107: pshicmp :=
>> ipv6PseudoHeader(c.LocalAddr().(*net.IPAddr).IP, dst.IP, 58)
>> On 2013/11/02 03:34:14, dfc wrote:
>> > 58 is a magic number, is it the IPv6 ether type ?
>
>
>> no, that's a protocol number.
>
>
>> interesting, why do you think that ipv6 upper layer stuff needs
>
> ethernet types
>>
>> to calculate its checksum? iirc, ietf derivarables basically implement
>> "lower-layer independency".
>
>
> I was mistaken, I should have said protocol number, not ether type.
>
>
>
> https://codereview.appspot.com/15020047/

[mikio, 2013-11-02 07:48:20]

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

go.net/ipv6: add ICMP checksum test

This CL enables testing raw ICMP packets with and without a setting of
ICMP checksum field.

Actually it's just for Linux because almost all BSD variants never
allow us to touch the checksum field of ICMP but Linux does when we use
the value 255 for the socket level.

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