|
|
Descriptionnet: make IP address selection work correctly on IPv6-only kernel
Update issue 3610
Update issue 5267
Update issue 5707
Patch Set 1 #Patch Set 2 : diff -r ac410fc1fe6c https://code.google.com/p/go #Patch Set 3 : diff -r ac410fc1fe6c https://code.google.com/p/go #
Total comments: 7
Patch Set 4 : diff -r cfaef1dc8164 https://code.google.com/p/go #
Total comments: 6
Patch Set 5 : diff -r be6c9de7b802 https://code.google.com/p/go #
Total comments: 10
Patch Set 6 : diff -r 8b61a4ec6d1e https://code.google.com/p/go #
Total comments: 4
Patch Set 7 : diff -r 19591dcb81c6 https://code.google.com/p/go #
MessagesTotal messages: 13
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
Sign in to reply to this message.
Can you update the CL description with an explanation of what this is doing? What was inaccurate before that is accurate now, and why? What is changing? On Sat, Jul 27, 2013 at 12:48 AM, <mikioh.mikioh@gmail.com> wrote: > Reviewers: golang-dev1, > > Message: > 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 > > > Description: > net: more accurate IP address selection on single, dual stack kernels > > Update issue 3610 > Update issue 5267 > Update issue 5707 > > Please review this at https://codereview.appspot.**com/11958043/<https://codereview.appspot.com/119... > > Affected files: > M src/pkg/net/ipsock.go > M src/pkg/net/ipsock_plan9.go > M src/pkg/net/ipsock_posix.go > > > Index: src/pkg/net/ipsock.go > ==============================**==============================**======= > --- a/src/pkg/net/ipsock.go > +++ b/src/pkg/net/ipsock.go > @@ -8,10 +8,11 @@ > > import "time" > > -var supportsIPv6, supportsIPv4map bool > +var supportsIPv4, supportsIPv6, supportsIPv4map bool > > func init() { > sysInit() > + supportsIPv4 = probeIPv4Stack() > supportsIPv6, supportsIPv4map = probeIPv6Stack() > } > > @@ -41,23 +42,27 @@ > return nil > } > > -func anyaddr(x IP) IP { > - if x4 := x.To4(); x4 != nil { > - return x4 > - } > - if supportsIPv6 { > - return x > +func anyaddr(ip IP) IP { > + if ip4 := ip.To4(); ip4 != nil && supportsIPv4 { > + return ip4 > + } else if len(ip) == IPv6len && supportsIPv6 { > + return ip > } > return nil > } > > -func ipv4only(x IP) IP { return x.To4() } > +func ipv4only(ip IP) IP { > + if ip.To4() != nil && supportsIPv4 { > + return ip > + } > + return nil > +} > > -func ipv6only(x IP) IP { > +func ipv6only(ip IP) IP { > // Only return addresses that we can use > // with the kernel's IPv6 addressing modes. > - if len(x) == IPv6len && x.To4() == nil && supportsIPv6 { > - return x > + if len(ip) == IPv6len && ip.To4() == nil && supportsIPv6 { > + return ip > } > return nil > } > Index: src/pkg/net/ipsock_plan9.go > ==============================**==============================**======= > --- a/src/pkg/net/ipsock_plan9.go > +++ b/src/pkg/net/ipsock_plan9.go > @@ -12,6 +12,10 @@ > "syscall" > ) > > +func probeIPv4Stack() bool { > + return true > +} > + > // probeIPv6Stack returns two boolean values. If the first boolean > // value is true, kernel supports basic IPv6 functionality. If the > // second boolean value is true, kernel supports IPv6 IPv4-mapping. > Index: src/pkg/net/ipsock_posix.go > ==============================**==============================**======= > --- a/src/pkg/net/ipsock_posix.go > +++ b/src/pkg/net/ipsock_posix.go > @@ -13,6 +13,15 @@ > "time" > ) > > +func probeIPv4Stack() bool { > + s, err := syscall.Socket(syscall.AF_**INET, syscall.SOCK_STREAM, > syscall.IPPROTO_TCP) > + if err != nil { > + return false > + } > + closesocket(s) > + return true > +} > + > // Should we try to use the IPv4 socket interface if we're > // only dealing with IPv4 sockets? As long as the host system > // understands IPv6, it's okay to pass IPv4 addresses to the IPv6 > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
Thanks for this proposal, here are some comments. I agree with Brad that the description might be better written as net: check if the host has a functioning IPv4 stack. https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:10: i'd like to see a small comment about each of these, ie var ( // supportsIPv4 indicates the host has a functioning IPv4 network stack. supportsIPv4 bool // supportsIPv4map indicates the host transparently maps IPv4 address space // into iPV6 space at [magic prefix] supportsIPv4map // supportsIPv6 ... ... ) https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:46: if ip4 := ip.To4(); ip4 != nil && supportsIPv4 { shouldn't the supportsIPv4 condition com before ? ie, there is no point in seeing if ip is an ipv4 address if this system cannot use it? if supportsIPv4 { if ip := ip.To4(); ip != nil { return ip } } if supportsIPv6 { ... } return nil https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:56: return ip please add a comment similar to the ipv6only below also consider rearranging the order of the check.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bradfitz@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
thanks both for the comments, ptal. https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:10: On 2013/07/28 00:37:44, dfc wrote: > i'd like to see a small comment about each of these, ie > > var ( > // supportsIPv4 indicates the host has a functioning IPv4 network stack. > supportsIPv4 bool > > // supportsIPv4map indicates the host transparently maps IPv4 address space > // into iPV6 space at [magic prefix] > supportsIPv4map > > // supportsIPv6 ... > ... > ) Done. https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:46: if ip4 := ip.To4(); ip4 != nil && supportsIPv4 { On 2013/07/28 00:37:44, dfc wrote: > shouldn't the supportsIPv4 condition com before ? ie, there is no point in > seeing if ip is an ipv4 address if this system cannot use it? > > if supportsIPv4 { > if ip := ip.To4(); ip != nil { > return ip > } > } > if supportsIPv6 { > ... > } > return nil Done. https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:56: return ip On 2013/07/28 00:37:44, dfc wrote: > please add a comment similar to the ipv6only below > > also consider rearranging the order of the check. Done. https://codereview.appspot.com/11958043/diff/6001/src/pkg/net/ipsock.go#newco... src/pkg/net/ipsock.go:56: return ip Also this should return a surely 4-byte stuff.
Sign in to reply to this message.
LGTM. Please wait for bradfitz.
Sign in to reply to this message.
https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:16: // supportsIPv6 reports whether the plaffrom supports IPv6 s/plaffrom/platform/ https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:20: // supportsIPv4map reports whether the plafform supports s/plaffrom/platform/ https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:21: // tranport layer interoperations between IPv4 and IPv6 using s/tranport/transport/
Sign in to reply to this message.
Thanks. https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:16: // supportsIPv6 reports whether the plaffrom supports IPv6 On 2013/07/28 07:35:41, fvb wrote: > s/plaffrom/platform/ Done. https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:20: // supportsIPv4map reports whether the plafform supports On 2013/07/28 07:35:41, fvb wrote: > s/plaffrom/platform/ Done. https://codereview.appspot.com/11958043/diff/13001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:21: // tranport layer interoperations between IPv4 and IPv6 using On 2013/07/28 07:35:41, fvb wrote: > s/tranport/transport/ Done.
Sign in to reply to this message.
ping
Sign in to reply to this message.
https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:20: // supportsIPv4map reports whether the platform supports this comment is somewhat ambiguous about whether it's mapping IPv4 into IPv6 or IPv6 into IPv4. https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:21: // transport layer interoperations between IPv4 and IPv6 using interoperability https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:58: func anyaddr(ip IP) IP { can you document what this is supposed to be doing? https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_plan9.go File src/pkg/net/ipsock_plan9.go (right): https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_plan9.g... src/pkg/net/ipsock_plan9.go:16: return true add a TODO in here and probeIPv6Stack to implement this on plan9? https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_posix.go File src/pkg/net/ipsock_posix.go (right): https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_posix.g... src/pkg/net/ipsock_posix.go:18: if err != nil { I suppose the error could be something like EMFILE instead of EPROTONOSUPPORT or EAFNOSUPPORT, but seems unlikely in init.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:20: // supportsIPv4map reports whether the platform supports On 2013/08/01 21:44:12, bradfitz wrote: > this comment is somewhat ambiguous about whether it's mapping IPv4 into IPv6 or > IPv6 into IPv4. add references. https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:21: // transport layer interoperations between IPv4 and IPv6 using On 2013/08/01 21:44:12, bradfitz wrote: > interoperability Done. https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:58: func anyaddr(ip IP) IP { On 2013/08/01 21:44:12, bradfitz wrote: > can you document what this is supposed to be doing? Done. https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_plan9.go File src/pkg/net/ipsock_plan9.go (right): https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_plan9.g... src/pkg/net/ipsock_plan9.go:16: return true On 2013/08/01 21:44:12, bradfitz wrote: > add a TODO in here and probeIPv6Stack to implement this on plan9? Done. https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_posix.go File src/pkg/net/ipsock_posix.go (right): https://codereview.appspot.com/11958043/diff/21001/src/pkg/net/ipsock_posix.g... src/pkg/net/ipsock_posix.go:18: if err != nil { On 2013/08/01 21:44:12, bradfitz wrote: > I suppose the error could be something like EMFILE instead of EPROTONOSUPPORT or > EAFNOSUPPORT, but seems unlikely in init. > unlikely but perhaps other error happens and still IPv4 is kinda lifeline. for now i"ll take either EPROTONOSUPPORT or EAFNOSUPPORT as a signal of the lack of IPv4 functionality.
Sign in to reply to this message.
LGTM but see comments on comments https://codereview.appspot.com/11958043/diff/30001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/30001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:25: // See RFC 4291, RFC 4038 and RFC 3493 for more inforamtion typo: information. But all those words don't add anything. You can just say: "See RFC 4291, 4038 and 3493." Actually the whole comment is long. You can probably just say: // supportsIPv4map reports whether the platform supports // mapping an IPv4 address inside an IPv6 address. // See RFC 4291, 4038 and 3493. At least, those sound the same to me, but one has fewer words. https://codereview.appspot.com/11958043/diff/30001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:64: // kernel configuraiton. typo: configuration But this comment doesn't say much. It just says "anyaddr returns a thing". I'd say something about whether this ever returns nil.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=7c06f390d1ad *** net: make IP address selection work correctly on IPv6-only kernel Update issue 3610 Update issue 5267 Update issue 5707 R=golang-dev, bradfitz, dave, fvbommel CC=golang-dev https://codereview.appspot.com/11958043 https://codereview.appspot.com/11958043/diff/30001/src/pkg/net/ipsock.go File src/pkg/net/ipsock.go (right): https://codereview.appspot.com/11958043/diff/30001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:25: // See RFC 4291, RFC 4038 and RFC 3493 for more inforamtion On 2013/08/02 17:32:40, bradfitz wrote: > Actually the whole comment is long. You can probably just say: > > // supportsIPv4map reports whether the platform supports > // mapping an IPv4 address inside an IPv6 address. > // See RFC 4291, 4038 and 3493. > > At least, those sound the same to me, but one has fewer words. Thanks, will go w/ yours + "mapping ... at transport layer protocols". https://codereview.appspot.com/11958043/diff/30001/src/pkg/net/ipsock.go#newc... src/pkg/net/ipsock.go:64: // kernel configuraiton. On 2013/08/02 17:32:40, bradfitz wrote: > typo: configuration > > But this comment doesn't say much. It just says "anyaddr returns a thing". I'd > say something about whether this ever returns nil. Done.
Sign in to reply to this message.
|