code review 7304091: go.net/ipv4: fix sprious lookahead on IPConn-based Pack...

go.net/ipv4: fix sprious lookahead on IPConn-based PacketConn

Also improves test coverage for both payload and datagram I/O.

[mikio, 2013-02-13 10:31: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

[dave_cheney.net, 2013-02-15 07:27:27]

Strangely coverage is unaffected by this change, maybe those paths are already
covered.

[dave_cheney.net, 2013-02-15 07:50:30]

LGTM. Some minor style comments if you feel like addressing them. I think this
is good, but i'd like to see a second opinion before you commit.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go
File ipv4/mockicmp_test.go (right):

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode40
ipv4/mockicmp_test.go:40: return nil, syscall.EINVAL
How likely is this ?

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode42
ipv4/mockicmp_test.go:42: b := make([]byte, 4)
For some reason I have developed an aversion to using make like this. Consider
this alternative

b := []byte {
    byte(m.Type),
    byte(m.Code), 
    0, // checksum (upper half)
    0, // checksum (lower half)
}

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode58
ipv4/mockicmp_test.go:58: for i := 0; i < msglen-1; i += 2 {
i'm uncomfortable with all the msglen-1's, what about

msglen := len(b) -1
...
if msglen&1 == 0 { 
    ...
}

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode80
ipv4/mockicmp_test.go:80: if msglen-4 > 0 {
what about msglen > 3 ?

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcod...
ipv4/mockicmp_test.go:122: if bodylen-4 > 0 {
same as above

https://codereview.appspot.com/7304091/diff/2007/ipv4/mocktransponder_test.go
File ipv4/mocktransponder_test.go (right):

https://codereview.appspot.com/7304091/diff/2007/ipv4/mocktransponder_test.go...
ipv4/mocktransponder_test.go:51: wh := &ipv4.Header{}
Please use the constant literal form here.

https://codereview.appspot.com/7304091/diff/2007/ipv4/multicast_test.go
File ipv4/multicast_test.go (right):

https://codereview.appspot.com/7304091/diff/2007/ipv4/multicast_test.go#newco...
ipv4/multicast_test.go:91: for i, toggle := range []bool{true, false, true} {
nice

[mikio, 2013-02-16 03:20:32]

PTAL

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go
File ipv4/mockicmp_test.go (right):

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode40
ipv4/mockicmp_test.go:40: return nil, syscall.EINVAL
On 2013/02/15 07:50:30, dfc wrote:
> How likely is this ?

not so often.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode42
ipv4/mockicmp_test.go:42: b := make([]byte, 4)
On 2013/02/15 07:50:30, dfc wrote:
> For some reason I have developed an aversion to using make like this. Consider
> this alternative
> 
> b := []byte {
>     byte(m.Type),
>     byte(m.Code), 
>     0, // checksum (upper half)
>     0, // checksum (lower half)
> }

can you share the reason why for this with us?
well, just curious.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode58
ipv4/mockicmp_test.go:58: for i := 0; i < msglen-1; i += 2 {
On 2013/02/15 07:50:30, dfc wrote:
> i'm uncomfortable with all the msglen-1's, what about
> 
> msglen := len(b) -1
> ...
> if msglen&1 == 0 { 
>     ...
> }

Done.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode80
ipv4/mockicmp_test.go:80: if msglen-4 > 0 {
On 2013/02/15 07:50:30, dfc wrote:
> what about msglen > 3 ?

msglen > 4 is fine.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcod...
ipv4/mockicmp_test.go:122: if bodylen-4 > 0 {
On 2013/02/15 07:50:30, dfc wrote:
> same as above

Done.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mocktransponder_test.go
File ipv4/mocktransponder_test.go (right):

https://codereview.appspot.com/7304091/diff/2007/ipv4/mocktransponder_test.go...
ipv4/mocktransponder_test.go:51: wh := &ipv4.Header{}
On 2013/02/15 07:50:30, dfc wrote:
> Please use the constant literal form here.

Done.

[dave_cheney.net, 2013-02-16 03:53:58]

LGTM. Thank you.

Please ignore my earlier comments about code coverage, something may have gone
wrong with my copy of gocov.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go
File ipv4/mockicmp_test.go (right):

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode40
ipv4/mockicmp_test.go:40: return nil, syscall.EINVAL
On 2013/02/16 03:20:32, mikio wrote:
> On 2013/02/15 07:50:30, dfc wrote:
> > How likely is this ?
> 
> not so often.

What I meant to say is this could only happen it someone did

var m *icmpMessage
b, err := m.Marshal()

I think this check is good to be safe, but there are soooo many other places in
Go where we don't check.

https://codereview.appspot.com/7304091/diff/2007/ipv4/mockicmp_test.go#newcode42
ipv4/mockicmp_test.go:42: b := make([]byte, 4)
On 2013/02/16 03:20:32, mikio wrote:
> On 2013/02/15 07:50:30, dfc wrote:
> > For some reason I have developed an aversion to using make like this.
Consider
> > this alternative
> > 
> > b := []byte {
> >     byte(m.Type),
> >     byte(m.Code), 
> >     0, // checksum (upper half)
> >     0, // checksum (lower half)
> > }
> 
> can you share the reason why for this with us?
> well, just curious.

When I see b := make([]byte, 4)

it doesn't tell me what b is used for. Sure, most of the time it is filled out
directly below, but sometimes not.

In this instance you can take the opportunity to avoid assigning to the memory
twice, and be more descriptive in the process.

[mikio, 2013-02-16 04:01:11]

Thank you for your review.

On Sat, Feb 16, 2013 at 12:53 PM,  <dave@cheney.net> wrote:

> What I meant to say is this could only happen it someone did
>
> var m *icmpMessage
> b, err := m.Marshal()
>
> I think this check is good to be safe, but there are soooo many other
> places in Go where we don't check.

yup, go will crash when the programmer made a blunder.
and this is just for testing.

> When I see b := make([]byte, 4)
>
> it doesn't tell me what b is used for. Sure, most of the time it is
> filled out directly below, but sometimes not.
>
> In this instance you can take the opportunity to avoid assigning to the
> memory twice, and be more descriptive in the process.

I see, thanks for the sharing.

[mikio, 2013-02-16 04:02:13]

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

go.net/ipv4: fix sprious lookahead on IPConn-based PacketConn

Also improves test coverage for both payload and datagram I/O.

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