code review 4172045: net: improve IPv6 capability probe and IPv4/v6 dual-sta...

net: improve IPv6 capability probe and IPv4/v6 dual-stack hostname lookup

[mikio, 2011-02-14 10:43:53]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[mikio, 2011-02-14 11:54:54]

I just changed description to:
net: improve IPv6 capability probe and IPv4/v6 dual-stack hostname lookup

[rsc, 2011-02-14 14:38:52]

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go
File src/pkg/net/iprawsock.go (right):

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go#newc...
src/pkg/net/iprawsock.go:248: for i := 0; i < cap(addrs); i++ {
Looping over cap is almost always wrong.
See http://golang.org/doc/effective_go.html#slices
Good place for a range statement anyway.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go#newc...
src/pkg/net/iprawsock.go:252: if kernelSupportsIPv6() {
if kernelSupportsIPv6() || addr.To4() != nil {
    break
}
addr = nil

No need for the socket ugliness.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode21
src/pkg/net/ipsock.go:21: // AFAIK several operating system kernels which
contain KAME IPv6
Delete this comment.
The one above says the same thing.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode25
src/pkg/net/ipsock.go:25: // c.f.
<http://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02>
I don't see why this reference is relevant.
It is talking about sending IPv4 addresses in actual IPv6 network packets.
This code is about a kernel API, not a wire protocol.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode27
src/pkg/net/ipsock.go:27: s, err := syscall.Socket(syscall.AF_INET6,
syscall.SOCK_DGRAM, syscall.IPPROTO_UDP)
Why does this use SOCK_DGRAM instead of SOCK_STREAM?
I am worried about SOCK_DGRAM triggering the OS X firewall.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode33
src/pkg/net/ipsock.go:33: ra := &UDPAddr{IP: IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0xff, 0xff, 127, 0, 0, 1}}
ra := &UDPAddr{IP: IPv4(127,0,0,1)}
would work fine

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode34
src/pkg/net/ipsock.go:34: sa, oserr := (ra.toAddr()).sockaddr(syscall.AF_INET6)
why the extra parens around ra.toAddr() ?

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode39
src/pkg/net/ipsock.go:39: err = syscall.Connect(s, sa)
return syscall.Connect(s, sa) == 0

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode226
src/pkg/net/ipsock.go:226: for i := 0; i < cap(addrs); i++ {
Make this a separate function, say

addr = firstSupportedAddr(addrs)

[mikio, 2011-02-14 15:32:36]

Thank you for your review.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go
File src/pkg/net/iprawsock.go (right):

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go#newc...
src/pkg/net/iprawsock.go:248: for i := 0; i < cap(addrs); i++ {
> Looping over cap is almost always wrong.

My bad, thank you for the suggestion.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go#newc...
src/pkg/net/iprawsock.go:252: if kernelSupportsIPv6() {
On 2011/02/14 14:38:52, rsc wrote:
> if kernelSupportsIPv6() || addr.To4() != nil {
>     break
> }
> addr = nil
> 
> No need for the socket ugliness.

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode21
src/pkg/net/ipsock.go:21: // AFAIK several operating system kernels which
contain KAME IPv6
On 2011/02/14 14:38:52, rsc wrote:
> Delete this comment.
> The one above says the same thing.

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode25
src/pkg/net/ipsock.go:25: // c.f.
<http://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02>
On 2011/02/14 14:38:52, rsc wrote:
> I don't see why this reference is relevant.
> It is talking about sending IPv4 addresses in actual IPv6 network packets.
> This code is about a kernel API, not a wire protocol.

You are right, removed.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode27
src/pkg/net/ipsock.go:27: s, err := syscall.Socket(syscall.AF_INET6,
syscall.SOCK_DGRAM, syscall.IPPROTO_UDP)
On 2011/02/14 14:38:52, rsc wrote:
> Why does this use SOCK_DGRAM instead of SOCK_STREAM?

Just curious.

> I am worried about SOCK_DGRAM triggering the OS X firewall.

socketfilter stuff? I suppose it might be good even SOCK_DGRAM
but I will change to SOCK_STREAM.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode33
src/pkg/net/ipsock.go:33: ra := &UDPAddr{IP: IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0xff, 0xff, 127, 0, 0, 1}}
On 2011/02/14 14:38:52, rsc wrote:
> ra := &UDPAddr{IP: IPv4(127,0,0,1)}
> would work fine

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode34
src/pkg/net/ipsock.go:34: sa, oserr := (ra.toAddr()).sockaddr(syscall.AF_INET6)
On 2011/02/14 14:38:52, rsc wrote:
> why the extra parens around ra.toAddr() ?

My bad cacoethes, removed.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode39
src/pkg/net/ipsock.go:39: err = syscall.Connect(s, sa)
On 2011/02/14 14:38:52, rsc wrote:
> return syscall.Connect(s, sa) == 0

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode226
src/pkg/net/ipsock.go:226: for i := 0; i < cap(addrs); i++ {
On 2011/02/14 14:38:52, rsc wrote:
> Make this a separate function, say
> 
> addr = firstSupportedAddr(addrs)

Done.

[mikio, 2011-02-14 15:33:00]

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

Please take another look.

[mikio, 2011-02-14 16:08:02]

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

Please take another look.

[mikio, 2011-02-14 16:25:06]

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

Please take another look.

[mikio, 2011-02-14 17:37:33]

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go
File src/pkg/net/iprawsock.go (right):

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/iprawsock.go#newc...
src/pkg/net/iprawsock.go:248: for i := 0; i < cap(addrs); i++ {
On 2011/02/14 15:32:36, mikioh wrote:
> > Looping over cap is almost always wrong.
> 
> My bad, thank you for the suggestion.

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode25
src/pkg/net/ipsock.go:25: // c.f.
<http://tools.ietf.org/html/draft-itojun-v6ops-v4mapped-harmful-02>
On 2011/02/14 15:32:36, mikioh wrote:
> On 2011/02/14 14:38:52, rsc wrote:
> > I don't see why this reference is relevant.
> > It is talking about sending IPv4 addresses in actual IPv6 network packets.
> > This code is about a kernel API, not a wire protocol.
> 
> You are right, removed.

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode27
src/pkg/net/ipsock.go:27: s, err := syscall.Socket(syscall.AF_INET6,
syscall.SOCK_DGRAM, syscall.IPPROTO_UDP)
On 2011/02/14 15:32:36, mikioh wrote:
> On 2011/02/14 14:38:52, rsc wrote:
> > Why does this use SOCK_DGRAM instead of SOCK_STREAM?
> 
> Just curious.
> 
> > I am worried about SOCK_DGRAM triggering the OS X firewall.
> 
> socketfilter stuff? I suppose it might be good even SOCK_DGRAM
> but I will change to SOCK_STREAM.

Done.

http://codereview.appspot.com/4172045/diff/4001/src/pkg/net/ipsock.go#newcode34
src/pkg/net/ipsock.go:34: sa, oserr := (ra.toAddr()).sockaddr(syscall.AF_INET6)
On 2011/02/14 15:32:36, mikioh wrote:
> On 2011/02/14 14:38:52, rsc wrote:
> > why the extra parens around ra.toAddr() ?
> 
> My bad cacoethes, removed.

Done.

[mikio, 2011-02-14 17:37:48]

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

Please take another look.

[rsc, 2011-02-16 19:13:13]

http://codereview.appspot.com/4172045/diff/6005/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

http://codereview.appspot.com/4172045/diff/6005/src/pkg/net/ipsock.go#newcode41
src/pkg/net/ipsock.go:41: if kernelSupportsIPv6() || addr.To4() != nil {
This should be !preferIPv4.
You don't want to call that function for every address you're considering.

[mikio, 2011-02-16 19:29:54]

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

Please take another look.

[mikio, 2011-02-16 19:32:59]

http://codereview.appspot.com/4172045/diff/6005/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

http://codereview.appspot.com/4172045/diff/6005/src/pkg/net/ipsock.go#newcode41
src/pkg/net/ipsock.go:41: if kernelSupportsIPv6() || addr.To4() != nil {
On 2011/02/16 19:13:13, rsc wrote:
> This should be !preferIPv4.
> You don't want to call that function for every address you're considering.

Done.

[rsc, 2011-02-16 20:05:13]

LGTM

[rsc, 2011-02-16 20:05:52]

*** Submitted as cd903c0a1760 ***

net: more accurate IPv4-in-IPv6 API test

R=rsc
CC=golang-dev
http://codereview.appspot.com/4172045

Committer: Russ Cox <rsc@golang.org>