code review 12898045: net: use protocol number literals instead of names in r...

net: add minimal internet protocol number information base

This CL adds minimal information for supporting platforms that don't
have a complete list of internet protocol numbers.

Fixes issue 5344.

[mikio, 2013-08-21 08:22:49]

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

[bradfitz, 2013-08-21 14:56:51]

I think it was prettier before.

I'd prefer to see the net package have its own small map of some common
numbers as a fallback.



On Wed, Aug 21, 2013 at 1:22 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: use protocol number literals instead of names in raw IP tests
>
> Some platforms don't have a complete list of protocol numbers.
>
> Fixes issue 5344.
>
> Please review this at
https://codereview.appspot.**com/12898045/<https://codereview.appspot.com/128...
>
> Affected files:
>   M src/pkg/net/ipraw_test.go
>   M src/pkg/net/packetconn_test.go
>   M src/pkg/net/protoconn_test.go
>   M src/pkg/net/unicast_posix_**test.go
>
>
> Index: src/pkg/net/ipraw_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/ipraw_test.go
> +++ b/src/pkg/net/ipraw_test.go
> @@ -24,11 +24,11 @@
>  var resolveIPAddrTests = []resolveIPAddrTest{
>         {"ip", "127.0.0.1", &IPAddr{IP: IPv4(127, 0, 0, 1)}, nil},
>         {"ip4", "127.0.0.1", &IPAddr{IP: IPv4(127, 0, 0, 1)}, nil},
> -       {"ip4:icmp", "127.0.0.1", &IPAddr{IP: IPv4(127, 0, 0, 1)}, nil},
> +       {"ip4:1", "127.0.0.1", &IPAddr{IP: IPv4(127, 0, 0, 1)}, nil},
>
>         {"ip", "::1", &IPAddr{IP: ParseIP("::1")}, nil},
>         {"ip6", "::1", &IPAddr{IP: ParseIP("::1")}, nil},
> -       {"ip6:ipv6-icmp", "::1", &IPAddr{IP: ParseIP("::1")}, nil},
> +       {"ip6:58", "::1", &IPAddr{IP: ParseIP("::1")}, nil},
>
>         {"ip", "::1%en0", &IPAddr{IP: ParseIP("::1"), Zone: "en0"}, nil},
>         {"ip6", "::1%911", &IPAddr{IP: ParseIP("::1"), Zone: "911"}, nil},
> @@ -67,8 +67,8 @@
>         laddr string
>         raddr string
>  }{
> -       {"ip4:icmp", "0.0.0.0", "127.0.0.1"},
> -       {"ip6:ipv6-icmp", "::", "::1"},
> +       {"ip4:1", "0.0.0.0", "127.0.0.1"},
> +       {"ip6:58", "::", "::1"},
>  }
>
>  func TestConnICMPEcho(t *testing.T) {
> @@ -331,9 +331,9 @@
>         net   string
>         laddr *IPAddr
>  }{
> -       {"ip4:icmp", &IPAddr{IP: IPv4(127, 0, 0, 1)}},
> -       {"ip4:icmp", &IPAddr{}},
> -       {"ip4:icmp", nil},
> +       {"ip4:1", &IPAddr{IP: IPv4(127, 0, 0, 1)}},
> +       {"ip4:1", &IPAddr{}},
> +       {"ip4:1", nil},
>  }
>
>  func TestIPConnLocalName(t *testing.T) {
> @@ -359,7 +359,7 @@
>         }
>
>         raddr := &IPAddr{IP: IPv4(127, 0, 0, 10).To4()}
> -       c, err := DialIP("ip:tcp", &IPAddr{IP: IPv4(127, 0, 0, 1)}, raddr)
> +       c, err := DialIP("ip:6", &IPAddr{IP: IPv4(127, 0, 0, 1)}, raddr)
>         if err != nil {
>                 t.Fatalf("DialIP failed: %v", err)
>         }
> Index: src/pkg/net/packetconn_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/packetconn_test.**go
> +++ b/src/pkg/net/packetconn_test.**go
> @@ -27,7 +27,7 @@
>         addr2 func() string
>  }{
>         {"udp", strfunc("127.0.0.1:0"), strfunc("127.0.0.1:0")},
> -       {"ip:icmp", strfunc("127.0.0.1"), strfunc("127.0.0.1")},
> +       {"ip:1", strfunc("127.0.0.1"), strfunc("127.0.0.1")},
>         {"unixgram", testUnixAddr, testUnixAddr},
>  }
>
> Index: src/pkg/net/protoconn_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/protoconn_test.**go
> +++ b/src/pkg/net/protoconn_test.**go
> @@ -185,7 +185,7 @@
>         if err != nil {
>                 t.Fatalf("ResolveIPAddr failed: %v", err)
>         }
> -       c, err := ListenIP("ip4:icmp", la)
> +       c, err := ListenIP("ip4:1", la)
>         if err != nil {
>                 t.Fatalf("ListenIP failed: %v", err)
>         }
> Index: src/pkg/net/unicast_posix_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/unicast_posix_**test.go
> +++ b/src/pkg/net/unicast_posix_**test.go
> @@ -457,7 +457,7 @@
>         if ln, err := ListenUDP("udp", nil); err == nil {
>                 ln.Close()
>         }
> -       if ln, err := ListenIP("ip:icmp", nil); err == nil {
> +       if ln, err := ListenIP("ip:1", nil); err == nil {
>                 ln.Close()
>         }
>  }
>
>
> --
>
> ---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...
> .
>

[mikio, 2013-08-22 00:37:21]

PTAL

> I'd prefer to see the net package have its own small map of some common
> numbers as a fallback.

done.

[bradfitz, 2013-08-22 01:03:35]

LGTM

[mikio, 2013-08-22 01:33:42]

*** Submitted as https://code.google.com/p/go/source/detail?r=911d3644ba26 ***

net: add minimal internet protocol number information base

This CL adds minimal information for supporting platforms that don't
have a complete list of internet protocol numbers.

Fixes issue 5344.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/12898045

[brainman, 2013-08-22 01:46:29]

No test? How do we know that this works?

Alex

[mikio, 2013-08-22 02:31:45]

Good catch. Just sent CL 12966046.

On Thu, Aug 22, 2013 at 10:46 AM,  <alex.brainman@gmail.com> wrote:
> No test? How do we know that this works?
>
> Alex
>
> https://codereview.appspot.com/12898045/

[brainman, 2013-08-22 02:52:52]

https://codereview.appspot.com/12898045/diff/17001/src/pkg/net/lookup.go
File src/pkg/net/lookup.go (right):

https://codereview.appspot.com/12898045/diff/17001/src/pkg/net/lookup.go#newc...
src/pkg/net/lookup.go:21: "ipv6-icmp": 58, "IPV6-ICMP": 58, "IPv6-ICMP": 58,
I am not convinced that this table can replace windows getprotobyname. The
protocol number returned by getprotobyname would not be used directly by our
program, instead it would be passed into another Windows API. How do you know
that 58 means IPV6-ICMP? I would say it is wrong for us to assume that. If
getprotobyname says it does not know what number is for IPV6-ICMP, we should
take its word as is.

[mikio, 2013-08-22 03:12:37]

On Thu, Aug 22, 2013 at 11:52 AM,  <alex.brainman@gmail.com> wrote:

> I am not convinced that this table can replace windows getprotobyname.
> The protocol number returned by getprotobyname would not be used
> directly by our program, instead it would be passed into another Windows
> API.

It's just helping parseNetwork, and ResolveProtoAddr or Dial functions.
I didn't see the code "passed into another Windows" in the net package
and this CL doesn't say "replacing getprotobyname".

> How do you know that 58 means IPV6-ICMP? I would say it is wrong
> for us to assume that. If getprotobyname says it does not know what
> number is for IPV6-ICMP, we should take its word as is.

Sorry, but I don't understand.

[brainman, 2013-08-22 07:45:05]

On 2013/08/22 03:12:37, mikio wrote:
> 
> > How do you know that 58 means IPV6-ICMP? I would say it is wrong
> > for us to assume that. If getprotobyname says it does not know what
> > number is for IPV6-ICMP, we should take its word as is.
> 
> Sorry, but I don't understand.

Perhaps, I am wrong, but won't you going to pass 58 into some Windows socket API
expecting 58 means IPV6-ICMP? I wouldn't expect that to work, since you didn't
get 58 from getprotobyname.

Alex

[ioe, 2013-08-22 14:50:38]

The actual numbers are from 
http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml
You are basically just allowed to ADD to this list locally, if your OS is 
not up to date yet or you are a network researcher developing a new IP 
subprotocol.
But the mappings itself are valid on any system, as this is the job of IANA 
already.

Did you see a different number returned on some windows systems? Does it 
work correctly if you do?

On Thursday, August 22, 2013 9:45:05 AM UTC+2, brainman wrote:
>
> On 2013/08/22 03:12:37, mikio wrote: 
>
> > > How do you know that 58 means IPV6-ICMP? I would say it is wrong 
> > > for us to assume that. If getprotobyname says it does not know what 
> > > number is for IPV6-ICMP, we should take its word as is. 
>
> > Sorry, but I don't understand. 
>
> Perhaps, I am wrong, but won't you going to pass 58 into some Windows 
> socket API expecting 58 means IPV6-ICMP? I wouldn't expect that to work, 
> since you didn't get 58 from getprotobyname. 
>
> Alex 
>
> https://codereview.appspot.com/12898045/ 
>

[brainman, 2013-08-23 02:33:54]

On 2013/08/22 14:50:38, ioe wrote:
> 
> Did you see a different number returned on some windows systems? ...

I didn't look, I do not know. But where does it say that numbers will match
http://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml?
Instead, Microsoft suggest we use getprotobyname instead. And we ignore that
advice and make some assumptions.

But I don't use any of that code myself (and I don't understand how it works),
so I will stop my arguments.

Alex