code review 6431053: net: fix ResolveIPAddr for IPv6 address enclosed in squ...

net: fix ResolveIPAddr for IPv6 address enclosed in square brackets

Fixes issue 3837.

[mikio, 2012-07-21 13:28:42]

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

[adg1, 2012-07-22 00:58:12]

Should this be in parseIPv6 instead?

Is [hostname] valid? Shouldn't ParseIP("[::]") work? Not clear to me that
it does.

On Saturday, July 21, 2012, wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> 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: fix ResolveIPAddr for IPv6 address enclosed in square brackets
>
> Fixes issue 3837.
>
> Please review this at
http://codereview.appspot.com/**6431053/<http://codereview.appspot.com/6431053/>
>
> Affected files:
>   M src/pkg/net/ipraw_test.go
>   M src/pkg/net/iprawsock.go
>
>
> Index: src/pkg/net/ipraw_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/ipraw_test.go
> +++ b/src/pkg/net/ipraw_test.go
> @@ -14,6 +14,27 @@
>         "time"
>  )
>
> +var resolveIPAddrTests = []struct {
> +       net  string
> +       addr string
> +}{
> +       {"ip", "0.0.0.0"},
> +       {"ip", "[::]"},
> +       {"ip", "::"},
> +       {"ip4", "0.0.0.0"},
> +       {"ip6", "[::]"},
> +       {"ip6", "::"},
> +}
> +
> +func TestResolveIPAddr(t *testing.T) {
> +       for _, tt := range resolveIPAddrTests {
> +               _, err := ResolveIPAddr(tt.net, tt.addr)
> +               if err != nil {
> +                       t.Fatalf("ResolveIPAddr(%q, %q) failed: %v",
> tt.net, tt.addr, err)
> +               }
> +       }
> +}
> +
>  var icmpTests = []struct {
>         net   string
>         laddr string
> Index: src/pkg/net/iprawsock.go
> ==============================**==============================**=======
> --- a/src/pkg/net/iprawsock.go
> +++ b/src/pkg/net/iprawsock.go
> @@ -46,6 +46,10 @@
>                 if net != "" && net[len(net)-1] == '6' {
>                         filter = ipv6only
>                 }
> +               // Can put brackets around host ...
> +               if len(host) > 0 && host[0] == '[' && host[len(host)-1] ==
> ']' {
> +                       host = host[1 : len(host)-1]
> +               }
>                 // Not an IP address.  Try as a DNS name.
>                 addrs, err1 := LookupHost(host)
>                 if err1 != nil {
>
>
>

[mikio, 2012-07-22 03:00:48]

Thank you for the comment. I realized that I put bracket remove lines
in wrong place, that should stay just before ParseIP in hostToIP.

On Sun, Jul 22, 2012 at 9:58 AM, Andrew Gerrand <adg@google.com> wrote:

> Should this be in parseIPv6 instead?

I guess, code itself says IP and IPAddr are different things. IP represents
a plain IP address and IPAddr represents something like IP address literal
in raw IP socket address, something like:

IP = IPv6Address
IPAddr = IPLiteralInRawIPSock ":" Port
IPLiteralInRawIPSock = "[" IPv6address / Name "]"

This looks similar to the relationship btw IPv6 address and literal IPv6
address in URL.

> Is [hostname] valid? Shouldn't ParseIP("[::]") work? Not clear to me that it
> does.

-- Mikio

[mikio, 2012-07-22 03:01:45]

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

Please take another look.

[adg, 2012-07-22 22:32:51]

This looks OK to me, but I'd like someone else familiar with net to take a look
also.

[pmarks, 2012-07-24 23:42:24]

On Sunday, July 22, 2012 3:32:51 PM UTC-7, Andrew Gerrand wrote:
>
> This looks OK to me, but I'd like someone else familiar with net to take 
> a look also. 
>
> http://codereview.appspot.com/6431053/ 
>

Generally, I would say that treating [f00::] as a host is a type mismatch. 
 This would be better handled by function that parses a string into a host 
and port, where the port number can be optional.

One example of this is Guava's HostAndPort class:
http://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/c...

There's also a Google-internal ParseHostOptionalPort() function for C++.

[rsc, 2012-07-30 00:05:19]

NOT LGTM

[::] is no more an IP address than 127.0.0.1:. The syntax for adding a port to
an IPv6 address is [addr]:port, but the [] are not part of the addr.

In addition to being consistent with the RFCs, this appears to be consistent
with other Unix programs. For example, on my Mac:

$ telnet '[::1]'
[::1]: nodename nor servname provided, or not known
$ telnet ::1
Trying ::1...
telnet: connect to address ::1: Connection refused
telnet: Unable to connect to remote host
$

[mikio, 2012-07-31 11:45:58]

On Mon, Jul 30, 2012 at 9:05 AM,  <rsc@golang.org> wrote:

> In addition to being consistent with the RFCs, this appears to be
> consistent with other Unix programs. For example, on my Mac:
>
> $ telnet '[::1]'
> [::1]: nodename nor servname provided, or not known
> $ telnet ::1
> Trying ::1...
> telnet: connect to address ::1: Connection refused
> telnet: Unable to connect to remote host
> $

Makes sense, will abandon.

[mikio, 2012-07-31 11:46:13]

*** Abandoned ***