code review 4749043: net: ParseCIDR returns IPNet instead of IPMask

net: ParseCIDR returns IPNet instead of IPMask

Note that this CL will break your existing code which uses
ParseCIDR.

This CL changes ParseCIDR("172.16.253.121/28") to return
the IP address "172.16.253.121", the network implied by the
network number "172.16.253.112" and mask "255.255.255.240".

[mikio, 2011-07-15 10:57:46]

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

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

[rsc, 2011-07-15 15:33:28]

I am not convinced.

If you write 192.168.254.254/24
you might have meant 192.168.254.0/24
or you might have meant 192.168.254.254/31
When the two halves are inconsistent,
why should we assume the second half
is the right one?

I can just as easily see other software
interpreting 192.168.254.254/24 as
meaning & with 255.255.255.0 and then
check if it's == 192.168.254.254 (impossible).

This really seems like an error to diagnose,
not something to silently reinterpret.

Russ

[rsc, 2011-07-15 15:41:17]

i checked with a networking expert here
and he says other libraries take your
interpretation too.  still seems like an error
to me but i am happy to be overruled.

will wait a day or two for others to weigh in.

[mikio, 2011-07-20 02:49:35]

ping.

On Sat, Jul 16, 2011 at 12:41 AM, Russ Cox <rsc@golang.org> wrote:

> i checked with a networking expert here
> and he says other libraries take your
> interpretation too.  still seems like an error
> to me but i am happy to be overruled.
>
> will wait a day or two for others to weigh in.

[rsc, 2011-07-20 13:17:37]

can you point to some other commonly-used software that does this?
i am still not convinced the majority agrees that this is a good idea.
i looked at the RFC and while it doesn't prohibit it, it also never uses
prefixes that contain bits not set in mask.

[borman, 2011-07-20 14:52:30]

This is a normal way to specify an IP address and netmask.

Old way:

    ifconfig exp0 192.168.1.2 --netmask 0xffffff00

New way

    ifconfig exp0 192.168.1.2/24

        -Paul

On Wed, Jul 20, 2011 at 6:17 AM, Russ Cox <rsc@golang.org> wrote:

> can you point to some other commonly-used software that does this?
> i am still not convinced the majority agrees that this is a good idea.
> i looked at the RFC and while it doesn't prohibit it, it also never uses
> prefixes that contain bits not set in mask.
>

[rsc, 2011-07-20 14:54:42]

On Wed, Jul 20, 2011 at 10:52, Paul Borman <borman@google.com> wrote:
> This is a normal way to specify an IP address and netmask.
> Old way:
>     ifconfig exp0 192.168.1.2 --netmask 0xffffff00
> New way
>     ifconfig exp0 192.168.1.2/24
>         -Paul

Okay but this code is not serving that purpose,
because it is returning 192.168.1.0 + 255.255.255.0,
so you can't recover the .2.

Russ

[rsc, 2011-07-20 14:57:26]

I found in RFC 4291 some support for your ifconfig command
but it still doesn't help with the ParseCIDR code.

   When writing both a node address and a prefix of that node address
   (e.g., the node's subnet prefix), the two can be combined as follows:

      the node address      2001:0DB8:0:CD30:123:4567:89AB:CDEF
      and its subnet number 2001:0DB8:0:CD30::/60

      can be abbreviated as 2001:0DB8:0:CD30:123:4567:89AB:CDEF/60

If we allow but do not mask, then callers all have to know to mask.
If we allow but do mask, then the ifconfig thing will silently fail.

I'm not thrilled with either.

[mikio, 2011-07-21 11:09:08]

On Wed, Jul 20, 2011 at 11:57 PM, Russ Cox <rsc@golang.org> wrote:

> If we allow but do not mask, then callers all have to know to mask.
> If we allow but do mask, then the ifconfig thing will silently fail.

How about this?

// ParseCIDR parses s as a CIDR notation IP address, address prefix
// and mask, like "192.168.100.1/24", "2001:DB8::/48", as defined in
// RFC 4632 and RFC 4291.
func ParseCIDR(s string) (ip IP, pfx IP, mask IPMask, err os.Error)

Adds a new return value pfx which describes its address prefix and
allows parsing both "fe80::/64" and "192.168.0.254/29" notations.

[borman, 2011-07-22 19:12:40]

I propose we change ParseCIDR to the following:

func ParseCIDR(s string) (ip IP, net IP, mask IPMask, err os.Error)

returning both the IP address and the network the IP address is part of as
x.y.z.w/n really provides you those three things.  If ParseCIDR does not do
this I will end up writing a new function because I do need to get back all
three pieces when processing certain networking configuration files.

    -Paul


On Wed, Jul 20, 2011 at 7:57 AM, Russ Cox <rsc@golang.org> wrote:

> I found in RFC 4291 some support for your ifconfig command
> but it still doesn't help with the ParseCIDR code.
>
>   When writing both a node address and a prefix of that node address
>   (e.g., the node's subnet prefix), the two can be combined as follows:
>
>      the node address      2001:0DB8:0:CD30:123:4567:89AB:CDEF
>      and its subnet number 2001:0DB8:0:CD30::/60
>
>      can be abbreviated as 2001:0DB8:0:CD30:123:4567:89AB:CDEF/60
>
> If we allow but do not mask, then callers all have to know to mask.
> If we allow but do mask, then the ifconfig thing will silently fail.
>
> I'm not thrilled with either.
>

[borman, 2011-07-22 19:13:47]

I see that is what Miko proposes :-)


On Fri, Jul 22, 2011 at 12:12 PM, Paul Borman <borman@google.com> wrote:

> I propose we change ParseCIDR to the following:
>
> func ParseCIDR(s string) (ip IP, net IP, mask IPMask, err os.Error)
>
> returning both the IP address and the network the IP address is part of as
> x.y.z.w/n really provides you those three things.  If ParseCIDR does not do
> this I will end up writing a new function because I do need to get back all
> three pieces when processing certain networking configuration files.
>
>     -Paul
>
>
> On Wed, Jul 20, 2011 at 7:57 AM, Russ Cox <rsc@golang.org> wrote:
>
>> I found in RFC 4291 some support for your ifconfig command
>> but it still doesn't help with the ParseCIDR code.
>>
>>   When writing both a node address and a prefix of that node address
>>   (e.g., the node's subnet prefix), the two can be combined as follows:
>>
>>      the node address      2001:0DB8:0:CD30:123:4567:89AB:CDEF
>>      and its subnet number 2001:0DB8:0:CD30::/60
>>
>>      can be abbreviated as 2001:0DB8:0:CD30:123:4567:89AB:CDEF/60
>>
>> If we allow but do not mask, then callers all have to know to mask.
>> If we allow but do mask, then the ifconfig thing will silently fail.
>>
>> I'm not thrilled with either.
>>
>
>

[mikio, 2011-08-07 15:29:08]

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

Please take another look.

[borman, 2011-08-07 16:22:06]

You probably should wait to hear from Russ before spending too much time on my
comments.  Russ may have additional/different/conflicting comments.  Russ
clearly had some thoughts on what an IPNet might be.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode572
src/pkg/net/ip.go:572: // address, a network mask, an address prefix and a
prefix
This is not quite right, an IP network is defined by a network and mask (e.g.,
1.2.3.0/24 or 1.2.3.0 and 255.255.255.0).  This structure is both an IP address
and the network to which the IP address belongs.  In any event, I would remove
the "It consists of ..." part and add a comment to each element.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode578
src/pkg/net/ip.go:578: PrefixLen int
While a CIDR mask has a prefix length, not all IP masks do.   I feel IPNet
should be a generic structure and not limited to CIDR only networks.  I would
also rename Prefix to Network.  I suppose the length could be -1 for non-CIDR
masks.

Also, shouldn't this be up with the other types?

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode585
src/pkg/net/ip.go:585: func ParseCIDR(s string) (*IPNet, os.Error) {
Changing the usage will require gofix to be updated as well.

We should consider having the signature being:

func ParseCIDR(s string) (IP, *IPNet, os.Error)

and having IPNet only define a network.  It will also make gofix easier for some
number of calls to ParseCIDR

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode587
src/pkg/net/ip.go:587: ip   IP
No need for this, just use := on line 597

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode589
src/pkg/net/ip.go:589: pfx  IP
No need for this, just use := on line 622

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode624
src/pkg/net/ip.go:624: // address prefix must not have any bits not in mask
This comment seems a bit off, now.  It made sense to explain why an error was
being returned in the original, but now the code seems self explanatory.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:52: {IP{0x20, 0x1, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0x1,
0x23, 0, 0x12, 0, 0x1}, "2001:db8::123:12:1"},
Why was this code changed and a new test added?  I cannot see the applicability
to what this CL is about.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:112: } else if err == nil && (!isEqual(out.IP, tt.out.IP)
|| !isEqual(out.Mask, tt.out.Mask) || !isEqual(out.Prefix, tt.out.Prefix) ||
out.PrefixLen != tt.out.PrefixLen) {
Shouldn't this be
if !reflect.DeepEqual(out, tt.out) {
    t.Errorf("ParseCIDR(%q) = %v; want %v", tt.in, out, tt.out)
}

[mikio, 2011-08-07 16:33:43]

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

Please take another look.

[mikio, 2011-08-07 16:44:01]

On Mon, Aug 8, 2011 at 1:22 AM,  <borman@google.com> wrote:

> You probably should wait to hear from Russ before spending too much time
> on my comments.  Russ may have additional/different/conflicting
> comments.  Russ clearly had some thoughts on what an IPNet might be.

Thank you for your suggestions. Agreed, this CL is just a starwman.

[mikio, 2011-08-10 15:57:23]

PTAL.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode572
src/pkg/net/ip.go:572: // address, a network mask, an address prefix and a
prefix
On 2011/08/07 16:22:06, borman wrote:
> This is not quite right, an IP network is defined by a network and mask (e.g.,
> 1.2.3.0/24 or 1.2.3.0 and 255.255.255.0).  This structure is both an IP
address
> and the network to which the IP address belongs.  In any event, I would remove
> the "It consists of ..." part and add a comment to each element.

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode578
src/pkg/net/ip.go:578: PrefixLen int
On 2011/08/07 16:22:06, borman wrote:
> While a CIDR mask has a prefix length, not all IP masks do.   I feel IPNet
> should be a generic structure and not limited to CIDR only networks.  I would
> also rename Prefix to Network.  I suppose the length could be -1 for non-CIDR
> masks.

address filtering w/ wildcards? certainly.

> Also, shouldn't this be up with the other types?

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode578
src/pkg/net/ip.go:578: PrefixLen int
On 2011/08/07 16:22:06, borman wrote:
> While a CIDR mask has a prefix length, not all IP masks do.   I feel IPNet
> should be a generic structure and not limited to CIDR only networks.  I would
> also rename Prefix to Network.  I suppose the length could be -1 for non-CIDR
> masks.
> 
> Also, shouldn't this be up with the other types?

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode585
src/pkg/net/ip.go:585: func ParseCIDR(s string) (*IPNet, os.Error) {
On 2011/08/07 16:22:06, borman wrote:
> Changing the usage will require gofix to be updated as well.
> 
> We should consider having the signature being:
> 
> func ParseCIDR(s string) (IP, *IPNet, os.Error)
> 
> and having IPNet only define a network.  It will also make gofix easier for
some
> number of calls to ParseCIDR
> 

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode587
src/pkg/net/ip.go:587: ip   IP
On 2011/08/07 16:22:06, borman wrote:
> No need for this, just use := on line 597

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode589
src/pkg/net/ip.go:589: pfx  IP
On 2011/08/07 16:22:06, borman wrote:
> No need for this, just use := on line 622

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip.go#newcode624
src/pkg/net/ip.go:624: // address prefix must not have any bits not in mask
On 2011/08/07 16:22:06, borman wrote:
> This comment seems a bit off, now.  It made sense to explain why an error was
> being returned in the original, but now the code seems self explanatory.

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:52: {IP{0x20, 0x1, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0x1,
0x23, 0, 0x12, 0, 0x1}, "2001:db8::123:12:1"},
On 2011/08/07 16:22:06, borman wrote:
> Why was this code changed and a new test added?  I cannot see the
applicability
> to what this CL is about.

just adjusted test table style like others.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:52: {IP{0x20, 0x1, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0x1,
0x23, 0, 0x12, 0, 0x1}, "2001:db8::123:12:1"},
On 2011/08/07 16:22:06, borman wrote:
> Why was this code changed and a new test added?  I cannot see the
applicability
> to what this CL is about.

Done.

http://codereview.appspot.com/4749043/diff/15002/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:112: } else if err == nil && (!isEqual(out.IP, tt.out.IP)
|| !isEqual(out.Mask, tt.out.Mask) || !isEqual(out.Prefix, tt.out.Prefix) ||
out.PrefixLen != tt.out.PrefixLen) {
On 2011/08/07 16:22:06, borman wrote:
> Shouldn't this be
> if !reflect.DeepEqual(out, tt.out) {
>     t.Errorf("ParseCIDR(%q) = %v; want %v", tt.in, out, tt.out)
> }

Done.

[mikio, 2011-08-10 15:57:50]

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

Please take another look.

[mikio, 2011-08-10 15:59:16]

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

Please take another look.

[rsc, 2011-08-16 20:17:06]

Sorry for the delay.

http://codereview.appspot.com/4749043/diff/26002/src/cmd/gofix/netparsecidr.go
File src/cmd/gofix/netparsecidr.go (right):

http://codereview.appspot.com/4749043/diff/26002/src/cmd/gofix/netparsecidr.g...
src/cmd/gofix/netparsecidr.go:11: var netparsecidrFix = fix{
I don't believe a gofix is warranted.
:= will take care of the simple cases,
and the hard cases require people to change their code.

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode42
src/pkg/net/ip.go:42: Network   IP     // network number
IP IP 

add three methods

// Net returns the network's base IP address,
// for example 192.168.0.0 for 192.168.2.3/16.
func (n *IPNet) Net() IP {

// Has reports whether the network includes ip.
func (n *IPNet) Has(ip IP) bool {

func (n *IPNet) String() string

I'd like a better name than Has if you have any ideas.

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode44
src/pkg/net/ip.go:44: PrefixLen int    // address prefix length
Delete.  Redundant with Mask.

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode582
src/pkg/net/ip.go:582: func ParseCIDR(s string) (IP, *IPNet, os.Error) {
ParseCIDR should return *IPNet, os.Error

[borman, 2011-08-16 20:29:24]

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode582
src/pkg/net/ip.go:582: func ParseCIDR(s string) (IP, *IPNet, os.Error) {
On 2011/08/16 20:17:06, rsc wrote:
> ParseCIDR should return *IPNet, os.Error

This does not make sense to me.  An IP address is not an IP Network and an IP
Network does not require an IP address.  You are getting two distinct things out
of this, an IP address and an IP Network.

[rsc, 2011-08-16 20:32:50]

> This does not make sense to me.  An IP address is not an IP Network and
> an IP Network does not require an IP address.  You are getting two
> distinct things out of this, an IP address and an IP Network.

I don't understand.

ParseCIDR("ip/mask")  returns &IPNet{ip, mask}

It's a 1:1 relationship between the input and output.

If you want the IP address, you use n.IP.
If you want the base network, you use n.Net().

[borman, 2011-08-16 20:48:02]

On Tue, Aug 16, 2011 at 3:32 PM, Russ Cox <rsc@golang.org> wrote:

> > This does not make sense to me.  An IP address is not an IP Network and
> > an IP Network does not require an IP address.  You are getting two
> > distinct things out of this, an IP address and an IP Network.
>
> I don't understand.
>
> ParseCIDR("ip/mask")  returns &IPNet{ip, mask}
>
> It's a 1:1 relationship between the input and output.
>
> If you want the IP address, you use n.IP.
> If you want the base network, you use n.Net().
>

ParseCIDR gives you more than two things, which is why this CL exists at
all.  It gives you an IP Address, the IP Network (host part being 0) and the
IP Mask.  An IP Network is defined by an IP address with the host part being
0 and an IP Mask (so you can figure out what the host part it).

1.2.3.4/24 -> An IP Address of 1.2.3.4 and an IP Network of 1.2.3.0/24 which
is composed of an Address (1.2.3.0) and a Mask (255.255.255.0).  If IPNet is
the definition of an IP Network then it should only have the latter two.  In
your case the n.Net() is insufficient, that does not tell me the IP network
unless I have the mask as well.  1.0.0.1/24 and 1.0.1.0/16 both would return
1.0.0.0 as the Net value but they are very much not part of the same
network.

I do not need an IP host address to have an IP Network address (which
includes the mask).

[rsc, 2011-08-16 20:52:27]

> ParseCIDR gives you more than two things, which is why this CL exists at
> all.  It gives you an IP Address, the IP Network (host part being 0) and the
> IP Mask.  An IP Network is defined by an IP address with the host part being
> 0 and an IP Mask (so you can figure out what the host part it).
> 1.2.3.4/24 -> An IP Address of 1.2.3.4 and an IP Network of 1.2.3.0/24 which
> is composed of an Address (1.2.3.0) and a Mask (255.255.255.0).  If IPNet is
> the definition of an IP Network then it should only have the latter two.  In
> your case the n.Net() is insufficient, that does not tell me the IP network
> unless I have the mask as well.  1.0.0.1/24 and 1.0.1.0/16 both would return
> 1.0.0.0 as the Net value but they are very much not part of the same
> network.

Yes, if you wanted to scribble the results somewhere else
you would want to save both n.Net() and n.Mask.
Why is that a problem?  Why are you scribbling anything
instead of just using n itself?

Russ

[borman, 2011-08-16 20:57:57]

On Tue, Aug 16, 2011 at 3:52 PM, Russ Cox <rsc@golang.org> wrote:

> > ParseCIDR gives you more than two things, which is why this CL exists at
> > all.  It gives you an IP Address, the IP Network (host part being 0) and
> the
> > IP Mask.  An IP Network is defined by an IP address with the host part
> being
> > 0 and an IP Mask (so you can figure out what the host part it).
> > 1.2.3.4/24 -> An IP Address of 1.2.3.4 and an IP Network of 1.2.3.0/24which
> > is composed of an Address (1.2.3.0) and a Mask (255.255.255.0).  If IPNet
> is
> > the definition of an IP Network then it should only have the latter two.
>  In
> > your case the n.Net() is insufficient, that does not tell me the IP
> network
> > unless I have the mask as well.  1.0.0.1/24 and 1.0.1.0/16 both would
> return
> > 1.0.0.0 as the Net value but they are very much not part of the same
> > network.
>
> Yes, if you wanted to scribble the results somewhere else
> you would want to save both n.Net() and n.Mask.
> Why is that a problem?  Why are you scribbling anything
> instead of just using n itself?
>
> Russ
>

IPNet should be

// An IPNet defines an IP Network
type IPNet struct {
    Network IP
    Mask IPMask
}

and that is it.  Adding "IP IP" doesn't make sense.  You don't need an IP
host address to define an IP Network.  ParseCIDR just happens to give you
both an IP address and an IP network (the latter including the mask).

[rsc, 2011-08-16 21:13:26]

Okay, I guess we have to let ParseCIDR return ip, net, err.
That said, the struct should still be

type IPNet struct {
    IP IP
    Mask IPMask
}

the IP field is the network IP.

[borman, 2011-08-16 21:23:42]

Okay, that is fine with me.

On Tue, Aug 16, 2011 at 4:13 PM, Russ Cox <rsc@golang.org> wrote:

> Okay, I guess we have to let ParseCIDR return ip, net, err.
> That said, the struct should still be
>
> type IPNet struct {
>    IP IP
>    Mask IPMask
> }
>
> the IP field is the network IP.
>

[mikio, 2011-08-17 03:40:34]

Thank you for the review, re-designing API.
I'll try to fix tonight or tomorrow.

+Paul: you know what I need, that's amazing, thx.

On Wed, Aug 17, 2011 at 6:13 AM, Russ Cox <rsc@golang.org> wrote:

> Okay, I guess we have to let ParseCIDR return ip, net, err.
> That said, the struct should still be
>
> type IPNet struct {
>    IP IP
>    Mask IPMask
> }
>
> the IP field is the network IP.

Just for clarification.

> src/cmd/gofix/netparsecidr.go:11: var netparsecidrFix = fix{
> I don't believe a gofix is warranted.
> := will take care of the simple cases,
> and the hard cases require people to change their code.

Agreed, but I have no good idea to above.
What's your suggestion, just warn() call makes sense to you?

[borman, 2011-08-17 13:53:08]

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode64
src/pkg/net/ip.go:64: copy(p, v4InV6Prefix)
Why are you switching how an IPv4 mask is represented?  Is this to differentiate
IPv6 masks with a prefix length of 96 or greater?

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode428
src/pkg/net/ip.go:428: func (n *IPNet) CanAggregate(ip IP) bool {
Perhaps "Contains" would be a better name here?  As a verb, aggregate means
"form or group into a class or cluster : [ intrans. ] the butterflies aggregate
in dense groups."

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode433
src/pkg/net/ip.go:433: if ip4 := ip.To4(); ip4 != nil {
if a = ip.To4(); a != nil {

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode435
src/pkg/net/ip.go:435: a = ip.To4()
Then drop this line

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode443
src/pkg/net/ip.go:443: quo := n.Mask.Len() / 8
What if the mask is not a proper prefix?  0xfffff0f0
Isn't this all just:

return n.IP.Equal(ip.Mask(n.Mask))

[rsc, 2011-08-17 14:00:36]

My suggestion wrt gofix is to do nothing.
The code will stop compiling.

Russ

[mikio, 2011-08-17 16:52:03]

http://codereview.appspot.com/4749043/diff/26002/src/cmd/gofix/netparsecidr.go
File src/cmd/gofix/netparsecidr.go (right):

http://codereview.appspot.com/4749043/diff/26002/src/cmd/gofix/netparsecidr.g...
src/cmd/gofix/netparsecidr.go:11: var netparsecidrFix = fix{
On 2011/08/16 20:17:06, rsc wrote:
> I don't believe a gofix is warranted.
> := will take care of the simple cases,
> and the hard cases require people to change their code.

okay, removed.

http://codereview.appspot.com/4749043/diff/26002/src/cmd/gofix/netparsecidr.g...
src/cmd/gofix/netparsecidr.go:11: var netparsecidrFix = fix{
On 2011/08/16 20:17:06, rsc wrote:
> I don't believe a gofix is warranted.
> := will take care of the simple cases,
> and the hard cases require people to change their code.

Done.

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode42
src/pkg/net/ip.go:42: Network   IP     // network number
On 2011/08/16 20:17:06, rsc wrote:
> IP IP 
> 
> add three methods
> 
> // Net returns the network's base IP address,
> // for example 192.168.0.0 for 192.168.2.3/16.
> func (n *IPNet) Net() IP {
> 
> // Has reports whether the network includes ip.
> func (n *IPNet) Has(ip IP) bool {
> 
> func (n *IPNet) String() string
> 
> I'd like a better name than Has if you have any ideas.

Done.

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode44
src/pkg/net/ip.go:44: PrefixLen int    // address prefix length
On 2011/08/16 20:17:06, rsc wrote:
> Delete.  Redundant with Mask.

Done.

http://codereview.appspot.com/4749043/diff/26002/src/pkg/net/ip.go#newcode582
src/pkg/net/ip.go:582: func ParseCIDR(s string) (IP, *IPNet, os.Error) {
On 2011/08/16 20:29:24, borman wrote:
> On 2011/08/16 20:17:06, rsc wrote:
> > ParseCIDR should return *IPNet, os.Error
> 
> This does not make sense to me.  An IP address is not an IP Network and an IP
> Network does not require an IP address.  You are getting two distinct things
out
> of this, an IP address and an IP Network.

Done.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode64
src/pkg/net/ip.go:64: copy(p, v4InV6Prefix)
On 2011/08/17 13:53:08, borman wrote:
> Why are you switching how an IPv4 mask is represented?  Is this to
differentiate
> IPv6 masks with a prefix length of 96 or greater?

Done.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode64
src/pkg/net/ip.go:64: copy(p, v4InV6Prefix)
On 2011/08/17 13:53:08, borman wrote:
> Why are you switching how an IPv4 mask is represented?  Is this to
differentiate
> IPv6 masks with a prefix length of 96 or greater?

previous IPMask.String fails the added new test 
TestIPMaskString, seems like a bug.

--- FAIL: net.TestIPMaskString (0.00 seconds)
	IPMask.String(ffff:ffff:ffff:fffe::) = `ffff:ffff:ffff:fffe::`, want `63`
	IPMask.String(ffff:ff80::) = `ffff:ff80::`, want `25`
	IPMask.String(ffff:ffff:ffff:ffff:ffff:ffff:ff00:ff00) =
`ffff:ffff:ffff:ffff:ffff:ffff:ff00:ff00`, want `255.0.255.0`

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode428
src/pkg/net/ip.go:428: func (n *IPNet) CanAggregate(ip IP) bool {
On 2011/08/17 13:53:08, borman wrote:
> Perhaps "Contains" would be a better name here?  As a verb, aggregate means
> "form or group into a class or cluster : [ intrans. ] the butterflies
aggregate
> in dense groups."

makes sense, thx.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode428
src/pkg/net/ip.go:428: func (n *IPNet) CanAggregate(ip IP) bool {
On 2011/08/17 13:53:08, borman wrote:
> Perhaps "Contains" would be a better name here?  As a verb, aggregate means
> "form or group into a class or cluster : [ intrans. ] the butterflies
aggregate
> in dense groups."

Done.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode433
src/pkg/net/ip.go:433: if ip4 := ip.To4(); ip4 != nil {
On 2011/08/17 13:53:08, borman wrote:
> if a = ip.To4(); a != nil {

Done.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode435
src/pkg/net/ip.go:435: a = ip.To4()
On 2011/08/17 13:53:08, borman wrote:
> Then drop this line

Done.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode443
src/pkg/net/ip.go:443: quo := n.Mask.Len() / 8
On 2011/08/17 13:53:08, borman wrote:
> What if the mask is not a proper prefix?  0xfffff0f0
> Isn't this all just:
> 
> return n.IP.Equal(ip.Mask(n.Mask))

good catch, thx.

http://codereview.appspot.com/4749043/diff/43001/src/pkg/net/ip.go#newcode443
src/pkg/net/ip.go:443: quo := n.Mask.Len() / 8
On 2011/08/17 13:53:08, borman wrote:
> What if the mask is not a proper prefix?  0xfffff0f0
> Isn't this all just:
> 
> return n.IP.Equal(ip.Mask(n.Mask))

Done.

[mikio, 2011-08-17 16:52:23]

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

Please take another look.

[rsc, 2011-08-17 17:07:36]

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode212
src/pkg/net/ip.go:212: if len(mask) == 16 && len(ip) == 4 && bytesEqual(ip[:12],
v4InV6Prefix) {
len(ip) == 4 so ip[:12] is going to crash.
I think allFF(mask[:12]) was correct.
There should be another if before the n != len(mask) check:

if len(mask) == 16 && len(ip) == 4 {
    ip = ip.To6()
}

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode377
src/pkg/net/ip.go:377: // Len returns the length of mask.  If the mask is a
sequence of
I don't know that this is a good idea.
It's hard to use Len by itself: you also need
to know whether mask is a IPv6 or IPv4 mask.
The String method really only works because
it gets paired with an IP address of the right length.
I don't see any uses of Len in the code.

Let's leave this out.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode427
src/pkg/net/ip.go:427: if n == nil {
In general you don't have to worry about nil receivers.
The caller shouldn't do that, and if it does, it will crash.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode431
src/pkg/net/ip.go:431: return n.IP.Mask(n.Mask).Equal(ip.Mask(n.Mask))
This is doing various allocations.  I would rather see a slightly
more complex implementation that never allocated.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode436
src/pkg/net/ip.go:436: // Net returns the network's base IP address,
Can delete this method now that code can just use n.IP.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode446
src/pkg/net/ip.go:446: if n == nil {
Can delete.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode621
src/pkg/net/ip.go:621: // number as known as address prefix and network mask
like
I can't parse this.  Suggested new comment:

// ParseCIDR parses s as a CIDR notation IP address and mask,
// like "192.168.100.1/16" or "2001:DB8::/48", as defined in
// RFC 4632 and RFC 4291.
//
// It returns the IP address and the network implied by the IP and mask.
// For example, ParseCIDR("192.168.100.1/16") returns the IP address
// 192.168.100.1 and the network 192.168.0.0/16.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:61: {nil, ""},
This is kind of weird.  I know it's what the old code did
but I would format nil as "<nil>" instead.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:135: {nil, ""},
Same.

[mikio, 2011-08-18 00:56:36]

On Thu, Aug 18, 2011 at 2:07 AM,  <rsc@golang.org> wrote:

> http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode377
> src/pkg/net/ip.go:377: // Len returns the length of mask.  If the mask
> is a sequence of
> I don't know that this is a good idea.
> It's hard to use Len by itself: you also need
> to know whether mask is a IPv6 or IPv4 mask.
> The String method really only works because
> it gets paired with an IP address of the right length.
> I don't see any uses of Len in the code.

I thought it would be nice when I have to write conventional routing
speakers such as OSPF/IS-IS/BGP/etc, because these guys carry
pairs of address prefix and prefix length to neighbors, not only
routing informations come from neighbors but also static entries
via configuration. It does not make sense to me that only prefix
length requires string to integer conversion with IPNet.

> Let's leave this out.

Well, can I put prefix length into IPNet struct again?

[rsc, 2011-08-18 17:15:56]

>> I don't know that this is a good idea.
>> It's hard to use Len by itself: you also need
>> to know whether mask is a IPv6 or IPv4 mask.
>> The String method really only works because
>> it gets paired with an IP address of the right length.
>> I don't see any uses of Len in the code.
>
> I thought it would be nice when I have to write conventional routing
> speakers such as OSPF/IS-IS/BGP/etc, because these guys carry
> pairs of address prefix and prefix length to neighbors, not only
> routing informations come from neighbors but also static entries
> via configuration. It does not make sense to me that only prefix
> length requires string to integer conversion with IPNet.
>
>> Let's leave this out.
>
> Well, can I put prefix length into IPNet struct again?

No, because then there's redundant information
to keep in sync.

I guess we need a method, but Len is not a good name
for it, since x.Len() and len(x) would return different numbers.

It bothers me that if IPMask is an IPv4 mask then you
get back 24 but if IPMask is an IPv6 mask then you get
back 120, so you need to know both the count and whether
this is an IPv4 mask or an IPv6 mask.  I am worried that
that will be a problem, but I don't know how to fix it.

For now how about:

// Count returns the number of leading ones in the mask.
// If the mask is not in the canonical form--ones followed by
// zeros--then Count returns -1.
func (m IPMask) Count() int

Russ

[borman, 2011-08-18 19:38:51]

On Thu, Aug 18, 2011 at 10:15 AM, Russ Cox <rsc@golang.org> wrote:

> >> I don't know that this is a good idea.
> >> It's hard to use Len by itself: you also need
> >> to know whether mask is a IPv6 or IPv4 mask.
> >> The String method really only works because
> >> it gets paired with an IP address of the right length.
> >> I don't see any uses of Len in the code.
> >
> > I thought it would be nice when I have to write conventional routing
> > speakers such as OSPF/IS-IS/BGP/etc, because these guys carry
> > pairs of address prefix and prefix length to neighbors, not only
> > routing informations come from neighbors but also static entries
> > via configuration. It does not make sense to me that only prefix
> > length requires string to integer conversion with IPNet.
> >
> >> Let's leave this out.
> >
> > Well, can I put prefix length into IPNet struct again?
>
> No, because then there's redundant information
> to keep in sync.
>
> I guess we need a method, but Len is not a good name
> for it, since x.Len() and len(x) would return different numbers.
>
> It bothers me that if IPMask is an IPv4 mask then you
> get back 24 but if IPMask is an IPv6 mask then you get
> back 120, so you need to know both the count and whether
> this is an IPv4 mask or an IPv6 mask.  I am worried that
> that will be a problem, but I don't know how to fix it.
>
> For now how about:
>
> // Count returns the number of leading ones in the mask.
> // If the mask is not in the canonical form--ones followed by
> // zeros--then Count returns -1.
> func (m IPMask) Count() int
>
> Russ
>

I spent some time talking with my brother, who is a leading member of the IETF
and was a core developer of the BSD networking stack.  The best name for
this would be PrefixLength.  That is what it is.

On the subject of ParseCIDR, he agrees with Mikio's first proposal of just
return the IP address and Mask.  The network number (which in this case is a
prefix) is simply ip.Mask(mask).

One thing he pointed out is a prefix is always a mask but a mask is not
always a prefix.

The actual expanded mask is probably more useful than the prefix length in
most instances.

Actually, I think PrefixLength should be a method on IPMask instead of
IPNet.

    -Paul

[mikio, 2011-08-19 09:18:13]

I'd like to take Paul's suggestion: PrefixLength on IPMask,
because it sounds natural to me.

// PrefixLength returns the number of leading ones in the mask.
// If the mask is not in the canonical form--ones followed by
// zeros--then PrefixLength returns -1.
func (m IPMask) PrefixLength() int

-- Mikio

[mikio, 2011-08-22 17:17:24]

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode212
src/pkg/net/ip.go:212: if len(mask) == 16 && len(ip) == 4 && bytesEqual(ip[:12],
v4InV6Prefix) {
On 2011/08/17 17:07:36, rsc wrote:
> len(ip) == 4 so ip[:12] is going to crash.
> I think allFF(mask[:12]) was correct.
> There should be another if before the n != len(mask) check:
> 
> if len(mask) == 16 && len(ip) == 4 {
>     ip = ip.To6()
> }

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode377
src/pkg/net/ip.go:377: // Len returns the length of mask.  If the mask is a
sequence of
On 2011/08/17 17:07:36, rsc wrote:
> I don't know that this is a good idea.
> It's hard to use Len by itself: you also need
> to know whether mask is a IPv6 or IPv4 mask.
> The String method really only works because
> it gets paired with an IP address of the right length.
> I don't see any uses of Len in the code.
> 
> Let's leave this out.

As we discussed: func (mask IPMask) PrefixLength() int

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode377
src/pkg/net/ip.go:377: // Len returns the length of mask.  If the mask is a
sequence of
On 2011/08/17 17:07:36, rsc wrote:
> I don't know that this is a good idea.
> It's hard to use Len by itself: you also need
> to know whether mask is a IPv6 or IPv4 mask.
> The String method really only works because
> it gets paired with an IP address of the right length.
> I don't see any uses of Len in the code.
> 
> Let's leave this out.

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode427
src/pkg/net/ip.go:427: if n == nil {
On 2011/08/17 17:07:36, rsc wrote:
> In general you don't have to worry about nil receivers.
> The caller shouldn't do that, and if it does, it will crash.

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode436
src/pkg/net/ip.go:436: // Net returns the network's base IP address,
On 2011/08/17 17:07:36, rsc wrote:
> Can delete this method now that code can just use n.IP.

removed.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode436
src/pkg/net/ip.go:436: // Net returns the network's base IP address,
On 2011/08/17 17:07:36, rsc wrote:
> Can delete this method now that code can just use n.IP.

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode446
src/pkg/net/ip.go:446: if n == nil {
On 2011/08/17 17:07:36, rsc wrote:
> Can delete.

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode621
src/pkg/net/ip.go:621: // number as known as address prefix and network mask
like
On 2011/08/17 17:07:36, rsc wrote:
> I can't parse this.  Suggested new comment:
> 
> // ParseCIDR parses s as a CIDR notation IP address and mask,
> // like "192.168.100.1/16" or "2001:DB8::/48", as defined in
> // RFC 4632 and RFC 4291.
> //
> // It returns the IP address and the network implied by the IP and mask.
> // For example, ParseCIDR("192.168.100.1/16") returns the IP address
> // 192.168.100.1 and the network 192.168.0.0/16.

Thanks.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip.go#newcode621
src/pkg/net/ip.go:621: // number as known as address prefix and network mask
like
On 2011/08/17 17:07:36, rsc wrote:
> I can't parse this.  Suggested new comment:
> 
> // ParseCIDR parses s as a CIDR notation IP address and mask,
> // like "192.168.100.1/16" or "2001:DB8::/48", as defined in
> // RFC 4632 and RFC 4291.
> //
> // It returns the IP address and the network implied by the IP and mask.
> // For example, ParseCIDR("192.168.100.1/16") returns the IP address
> // 192.168.100.1 and the network 192.168.0.0/16.

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:61: {nil, ""},
On 2011/08/17 17:07:36, rsc wrote:
> This is kind of weird.  I know it's what the old code did
> but I would format nil as "<nil>" instead.

Done.

http://codereview.appspot.com/4749043/diff/48001/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:135: {nil, ""},
On 2011/08/17 17:07:36, rsc wrote:
> Same.

Done.

[mikio, 2011-08-22 17:17:41]

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

Please take another look.

[rsc, 2011-08-22 18:01:11]

Looks good.  The only problem is the mask == v4InV6Prefix comparisons.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode212
src/pkg/net/ip.go:212: if len(ip) == 4 && len(mask) == 16 &&
bytesEqual(mask[:12], v4InV6Prefix) {
This should be allFF.  A mask equal to the IP prefix
would be a non-canonical mask.  It doesn't make any sense.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode266
src/pkg/net/ip.go:266: // String returns the string form of the IP address ip. 
If the
Why did this get reformatted?
Please unreformat.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode390
src/pkg/net/ip.go:390: if bytesEqual(mask[:12], v4InV6Prefix) {
I don't understand why this change was made.
It seems very wrong to compare the mask to v4InV6Prefix.
Comparing to allFF would make more sense but even then
it introduces an ambiguity.  I'd rather keep this as it was.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode413
src/pkg/net/ip.go:413: var n int
Same comment.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode432
src/pkg/net/ip.go:432: if len(ip) == 4 && len(n.Mask) == 16 &&
bytesEqual(n.Mask[:12], v4InV6Prefix) {
allFF

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode596
src/pkg/net/ip.go:596: // A ParseError represents a malformed text string and
the type of
Please unreformat.

[rsc, 2011-08-22 18:03:50]

> On the subject of ParseCIDR, he agrees with Mikio's first proposal of just
> return the IP address and Mask.  The network number (which in this case is a
> prefix) is simply ip.Mask(mask).

Returning the IPNet as a separate thing has the
important advantage of giving people ip.Mask(mask)
already computed.  I think many people would assume,
if we returned (IP, IPMask, os.Error), that the IP was
itself the network number and forget to mask.

> One thing he pointed out is a prefix is always a mask but a mask is not
> always a prefix.
> The actual expanded mask is probably more useful than the prefix length in
> most instances.

Right, this is why ParseCIDR in its current form returns
an IPMask instead of an int.

Russ

[borman, 2011-08-22 18:21:53]

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode212
src/pkg/net/ip.go:212: if len(ip) == 4 && len(mask) == 16 &&
bytesEqual(mask[:12], v4InV6Prefix) {
On 2011/08/22 18:01:11, rsc wrote:
> This should be allFF.  A mask equal to the IP prefix
> would be a non-canonical mask.  It doesn't make any sense.

See my comment below.  This does make sense.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode390
src/pkg/net/ip.go:390: if bytesEqual(mask[:12], v4InV6Prefix) {
On 2011/08/22 18:01:11, rsc wrote:
> I don't understand why this change was made.
> It seems very wrong to compare the mask to v4InV6Prefix.
> Comparing to allFF would make more sense but even then
> it introduces an ambiguity.  I'd rather keep this as it was.

I was confused by this at first, too, but I think Mikio is doing the right
thing.  A 4 byte IPv4 address is represented as a 16 byte value by prepending it
with v4InV6Prefix.  This is doing the same thing for IPv4 masks.  An IP mask is
an IP address, so I think this representation makes sense.

In the previous method, using all 1's, you can't tell the difference between a
/n IPv4 mask and a /(n+96) IPv6 mask.  For example, a /8 and /104 (in 16 byte
notation) both are:

    ffff:ffff:ffff:ffff:ffff:ffff:ff00:0000

With Mikio's change a /8 (in 16 byte notation) is:

    0000:0000:0000:0000:0000:ffff:ff00:0000

Now the ambiguity is removed.

[rsc, 2011-08-22 18:26:28]

> I was confused by this at first, too, but I think Mikio is doing the
> right thing.  A 4 byte IPv4 address is represented as a 16 byte value by
> prepending it with v4InV6Prefix.  This is doing the same thing for IPv4
> masks.  An IP mask is an IP address, so I think this representation
> makes sense.
>
> In the previous method, using all 1's, you can't tell the difference
> between a /n IPv4 mask and a /(n+96) IPv6 mask.  For example, a /8 and
> /104 (in 16 byte notation) both are:
>
>    ffff:ffff:ffff:ffff:ffff:ffff:ff00:0000
>
> With Mikio's change a /8 (in 16 byte notation) is:
>
>    0000:0000:0000:0000:0000:ffff:ff00:0000
>
> Now the ambiguity is removed.

Can you point to an RFC encouraging this representation?
It makes no sense to me: it's not a mask anymore.

You don't need to tell the difference between /n IPv4 and
/(n+96) IPv6.  They are the same.  You do need to tell
the difference between /n IPv4 and /n IPv6, and the only
way to do that is to know the mask size len(mask).  C'est la IP.

Russ

[mikio, 2011-08-22 18:30:46]

On Tue, Aug 23, 2011 at 3:01 AM,  <rsc@golang.org> wrote:

> http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode212
> src/pkg/net/ip.go:212: if len(ip) == 4 && len(mask) == 16 &&
> bytesEqual(mask[:12], v4InV6Prefix) {
> This should be allFF.  A mask equal to the IP prefix
> would be a non-canonical mask.  It doesn't make any sense.

But how could I find out the differentiation btw IPv6 IPv4-mapped
address and IPv6 address?

> http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode266
> src/pkg/net/ip.go:266: // String returns the string form of the IP
> address ip.  If the
> Why did this get reformatted?
> Please unreformat.

Oops, yup, forgot to revert it; I did check godoc behavior at same time
to investigate issue 2172, 2173.

[borman, 2011-08-22 18:41:38]

Perhaps we should go back to what Mikio first suggested then, return the IP
and IPMask represented by x.x.x.x/n or xxxx::xxxxx/n but not mask the IP
address as the original code was doing.  If they want the network they say:

    ip, mask, err := ParseCIDR(...)

    net := ip.Mask(mask),


On Mon, Aug 22, 2011 at 11:03 AM, Russ Cox <rsc@golang.org> wrote:

> > On the subject of ParseCIDR, he agrees with Mikio's first proposal of
> just
> > return the IP address and Mask.  The network number (which in this case
> is a
> > prefix) is simply ip.Mask(mask).
>
> Returning the IPNet as a separate thing has the
> important advantage of giving people ip.Mask(mask)
> already computed.  I think many people would assume,
> if we returned (IP, IPMask, os.Error), that the IP was
> itself the network number and forget to mask.
>
> > One thing he pointed out is a prefix is always a mask but a mask is not
> > always a prefix.
> > The actual expanded mask is probably more useful than the prefix length
> in
> > most instances.
>
> Right, this is why ParseCIDR in its current form returns
> an IPMask instead of an int.
>
> Russ
>

[rsc, 2011-08-22 18:50:08]

>> http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode212
>> src/pkg/net/ip.go:212: if len(ip) == 4 && len(mask) == 16 &&
>> bytesEqual(mask[:12], v4InV6Prefix) {
>> This should be allFF.  A mask equal to the IP prefix
>> would be a non-canonical mask.  It doesn't make any sense.
>
> But how could I find out the differentiation btw IPv6 IPv4-mapped
> address and IPv6 address?

I don't understand why it matters.  Can you give me a
specific ParseCIDR + IPNet.Contains sequence that
cares?

Specifically, I think that

&IPNet{IP{1,2,0,0}, IPMask{0xff,0xff,0,0}}
&IPNet{IP{0,0,0,0,0,0,0,0,0,0,0xff,0xff,1,2,0,0},
IPMask{0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0,0}}

are completely equivalent.  The first comes from
ParseCIDR("1.2.0.0/16") and the second from
ParseCIDR("::ffff:1.2.0.0/112").

Russ

[rsc, 2011-08-22 18:50:33]

On Mon, Aug 22, 2011 at 14:41, Paul Borman <borman@google.com> wrote:
> Perhaps we should go back to what Mikio first suggested then, return the IP
> and IPMask represented by x.x.x.x/n or xxxx::xxxxx/n but not mask the IP
> address as the original code was doing.  If they want the network they say:

No, I like having IPNet, if only for the cheaper Contains method.

Russ

[mikio, 2011-08-22 18:52:57]

On Tue, Aug 23, 2011 at 3:26 AM, Russ Cox <rsc@golang.org> wrote:

> Can you point to an RFC encouraging this representation?
> It makes no sense to me: it's not a mask anymore.

Hmm... what I really need is the prefix length of the network, ah, okay.
Does "func (n* IPNet) PrefixLength() int" make sense to both of you?

[rsc, 2011-08-22 18:55:46]

On Mon, Aug 22, 2011 at 14:52, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Tue, Aug 23, 2011 at 3:26 AM, Russ Cox <rsc@golang.org> wrote:
>
>> Can you point to an RFC encouraging this representation?
>> It makes no sense to me: it's not a mask anymore.
>
> Hmm... what I really need is the prefix length of the network, ah, okay.
> Does "func (n* IPNet) PrefixLength() int" make sense to both of you?

I grudgingly accept it, but I think it is not very useful
unless you also know whether this is an IPv4 and IPv6 mask:
/8 for IPv4 is not the same as /8 for IPv6.
I assume that the caller will be checking that out of band.

Russ

[mikio, 2011-08-22 19:06:56]

On Tue, Aug 23, 2011 at 3:50 AM, Russ Cox <rsc@golang.org> wrote:

> I don't understand why it matters.  Can you give me a
> specific ParseCIDR + IPNet.Contains sequence that
> cares?

I'd like to see ParseCIDR("abcd:2345::/127") returns the address,
the net and *the prefix length of the net", passes test cases like
below:

{"172.16.1.127/31", IPv4(172, 16, 1, 127), &IPNet{IPv4(172, 16, 1,
126), IPv4Mask(255, 255, 255, 254)}, 31, nil},
{"abcd:2345::/127", ParseIP("abcd:2345::"),
&IPNet{ParseIP("abcd:2345::"),
IPMask(ParseIP("ffff:ffff:ffff:ffff:ffff:ffff:ffff:fffe"))}, 127,
nil},

For now IPMask never knows the network number of the net, and is
internally stored 16-byte form thus returns incorrect prefix length.

If you can accept IPNet.PrefixLenght...

> I grudgingly accept it, but I think it is not very useful
> unless you also know whether this is an IPv4 and IPv6 mask:
> /8 for IPv4 is not the same as /8 for IPv6.
> I assume that the caller will be checking that out of band.

Ah, thanks, let me sleep on it.

-- Mikio

[mikio, 2011-08-24 07:35:14]

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode212
src/pkg/net/ip.go:212: if len(ip) == 4 && len(mask) == 16 &&
bytesEqual(mask[:12], v4InV6Prefix) {
On 2011/08/22 18:01:11, rsc wrote:
> This should be allFF.  A mask equal to the IP prefix
> would be a non-canonical mask.  It doesn't make any sense.

Done.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode266
src/pkg/net/ip.go:266: // String returns the string form of the IP address ip. 
If the
On 2011/08/22 18:01:11, rsc wrote:
> Why did this get reformatted?
> Please unreformat.

Done.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode390
src/pkg/net/ip.go:390: if bytesEqual(mask[:12], v4InV6Prefix) {
On 2011/08/22 18:01:11, rsc wrote:

> I don't understand why this change was made.
> It seems very wrong to compare the mask to v4InV6Prefix.
> Comparing to allFF would make more sense but even then
> it introduces an ambiguity.  I'd rather keep this as it was.

Okay, I will retreat from Mask.PreifxLength stuff but 
I'd like to go with func (n *IPNet) PrefixLength() int.
I understand that you did accept this grudgingly in 
previous review, hope it has been still kept.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode390
src/pkg/net/ip.go:390: if bytesEqual(mask[:12], v4InV6Prefix) {
On 2011/08/22 18:01:11, rsc wrote:
> I don't understand why this change was made.
> It seems very wrong to compare the mask to v4InV6Prefix.
> Comparing to allFF would make more sense but even then
> it introduces an ambiguity.  I'd rather keep this as it was.

Done.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode413
src/pkg/net/ip.go:413: var n int
On 2011/08/22 18:01:11, rsc wrote:
> Same comment.

Done.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode432
src/pkg/net/ip.go:432: if len(ip) == 4 && len(n.Mask) == 16 &&
bytesEqual(n.Mask[:12], v4InV6Prefix) {
On 2011/08/22 18:01:11, rsc wrote:
> allFF

Done.

http://codereview.appspot.com/4749043/diff/53001/src/pkg/net/ip.go#newcode596
src/pkg/net/ip.go:596: // A ParseError represents a malformed text string and
the type of
On 2011/08/22 18:01:11, rsc wrote:
> Please unreformat.

Done.

[mikio, 2011-08-24 07:35:32]

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

Please take another look.

[mikio, 2011-08-24 08:24:04]

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

Please take another look.

[mikio, 2011-08-24 10:44:05]

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

Please take another look.

[mikio, 2011-08-24 13:37:42]

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

Please take another look.

[rsc, 2011-08-24 16:47:13]

I am finally starting to understand some of this.

I made a mistake when I implemented IPMask originally:
I represented an IPMask as 16 byte slice always.
This makes the String method impossible to do correctly,
since the mask you get from parsing ::ffff:1.2.3.0/120
and the mask you get from parsing 1.2.3.0/24 have the
same 16-byte representation.  The current String
method subtracts 96 from numbers before printing them,
which means reprinting the first input will show as
::ffff:1.2.3.0/24 which is wrong.  The representation
is the core issue: an IPv4 IPMask should have len 4.
I have flagged the relevant lines below.

With that fix, I think things get clearer.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode63
src/pkg/net/ip.go:63: p := make(IPMask, IPv6len)
This is a mistake (mine).
To tell that something is an IPv4 mask it needs
to have length 4.

return IPMask{a,b,c,d}

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode175
src/pkg/net/ip.go:175: // If ip is not an IP address (it is the wrong length),
To16
unreformat

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode392
src/pkg/net/ip.go:392: // String returns the string representation of mask.  If
the mask
Please unreformat.

The line breaks in the original comment are where they are
so that you can skim from sentence to sentence easily.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode405
src/pkg/net/ip.go:405: if n >= 12*8 {
This should be n >= 0.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode406
src/pkg/net/ip.go:406: return itod(uint(n - 12*8))
This should be itod(uint(n))

[mikio, 2011-08-25 03:21:03]

> The representation is the core issue: an IPv4 IPMask should have len 4.

Sounds good, PTAL.

- PrefixLength is moved to IPMask as its method again
- Made IPMask.String and IPNet.String output format better,
   probably, I mean, a bit familiar to networking guys

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode175
src/pkg/net/ip.go:175: // If ip is not an IP address (it is the wrong length),
To16
On 2011/08/24 16:47:13, rsc wrote:
> unreformat

Done.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode392
src/pkg/net/ip.go:392: // String returns the string representation of mask.  If
the mask
On 2011/08/24 16:47:13, rsc wrote:
> Please unreformat.
> 
> The line breaks in the original comment are where they are
> so that you can skim from sentence to sentence easily.

Done.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode405
src/pkg/net/ip.go:405: if n >= 12*8 {
On 2011/08/24 16:47:13, rsc wrote:
> This should be n >= 0.

Done.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode406
src/pkg/net/ip.go:406: return itod(uint(n - 12*8))
On 2011/08/24 16:47:13, rsc wrote:
> This should be itod(uint(n))

Done.

[mikio, 2011-08-25 03:21:22]

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

Please take another look.

[borman, 2011-08-25 16:39:44]

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode214
src/pkg/net/ip.go:214: if len(ip) == IPv4len && len(mask) == IPv6len {
This allows an IPv6 mask to be applied to an IPv4 addresss.  Does that make
sense?  1.2.3.4/ffff:ff00:: will get displayed as 1.2.3.4/24 but will result in
0.0.0.0.  Maybe we shouldn't care, I don't know.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode357
src/pkg/net/ip.go:357: // as hexadeciaml.
An IPv6 Mask must be a single series of 1's followed by a single series of 0's. 
Anything else is an invalid IPv6 mask.  For IPv4 I see hex and dotted quad
equally.  Since this will only happen with a non-CIDR mask, which is the very
rare case, after all, I think the previous IP(mask).String() was probably just
fine.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode371
src/pkg/net/ip.go:371: bb.WriteString("0x")
move this above the for and remove the check

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode412
src/pkg/net/ip.go:412: return s + ", " + n.Mask.String()
This doesn't make sense to me.  It only is used when the mask is not a CIDR mask
but that is the time it can be most difficult to determine if something is a
mask or not "a.b.c.d, e.f.g.h" looks like 2 IP addresses while "a.b.c.d/e.f.g.h"
is more clear that the latter is a mask and not a second IP address.

[borman, 2011-08-25 16:45:51]

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode412
src/pkg/net/ip.go:412: return s + ", " + n.Mask.String()
On 2011/08/25 16:39:44, borman wrote:
> This doesn't make sense to me.  It only is used when the mask is not a CIDR
mask
> but that is the time it can be most difficult to determine if something is a
> mask or not "a.b.c.d, e.f.g.h" looks like 2 IP addresses while
"a.b.c.d/e.f.g.h"
> is more clear that the latter is a mask and not a second IP address.

Shouldn't this just be:

return n.IP.String() + "/" + n.Mask.String()

[rsc, 2011-08-25 21:19:46]

I think we are converging, but this CL has grown too large.
Let's regroup a bit.

First, send a CL with the 4->IPv4len and 16->IPv6len
changes only.  Then we can get that in.

Second, send a CL moving the decimal routines to
parse.go.  Then we can get that in.  Change
the itox call to itox(..., 1).

func itox(i uint, min int) string {
	// Assemble hexadecimal in reverse order.
	var b [32]byte
	bp := len(b)
	for ; i > 0 || min > 0; i /= 16 {
		bp--
		b[bp] = "0123456789abcdef"[byte(i%16)]
		min--
	}

	return string(b[bp:])
}

Third, send a CL with the changes to IPMask:
  * IPv4Mask returns a 4-length mask
  * String returns just unpunctuated hex.
  * add Size method.

// String returns the hexadecimal form of m, with no punctuation.
func (mask IPMask) String() string {
	s := ""
	for _, b := range m {
		s += itox(b, 2)
	}
}

// Size returns the number of leading ones and total bits in the mask.
// If the mask is not in the canonical form--ones followed by zeros--then
// Size returns 0, 0.
func (mask IPMask) Size() (ones, bits int)

Any necessary changes to ip.Mask can go here too.

Fourth, send a CL (or update this one) adding
IPNet and changing ParseCIDR.

I think this will speed things along because it
will be easier to focus on one thing at a time.

Russ

[mikio, 2011-08-25 21:53:33]

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode214
src/pkg/net/ip.go:214: if len(ip) == IPv4len && len(mask) == IPv6len {
On 2011/08/25 16:39:44, borman wrote:
> This allows an IPv6 mask to be applied to an IPv4 addresss.  Does that make
> sense?  1.2.3.4/ffff:ff00:: will get displayed as 1.2.3.4/24 but will result
in
> 0.0.0.0.  Maybe we shouldn't care, I don't know.

For now  there's no reason to accept an IPv6 mask for the IPv4 address.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode214
src/pkg/net/ip.go:214: if len(ip) == IPv4len && len(mask) == IPv6len {
On 2011/08/25 16:39:44, borman wrote:
> This allows an IPv6 mask to be applied to an IPv4 addresss.  Does that make
> sense?  1.2.3.4/ffff:ff00:: will get displayed as 1.2.3.4/24 but will result
in
> 0.0.0.0.  Maybe we shouldn't care, I don't know.

Done.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode357
src/pkg/net/ip.go:357: // as hexadeciaml.
On 2011/08/25 16:39:44, borman wrote:
> An IPv6 Mask must be a single series of 1's followed by a single series of
0's.
> Anything else is an invalid IPv6 mask.  

I think there's no consensus (and no convention) about that.

> after all, I think the previous IP(mask).String() was probably just
> fine.

Agreed.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode357
src/pkg/net/ip.go:357: // as hexadeciaml.
On 2011/08/25 16:39:44, borman wrote:
> An IPv6 Mask must be a single series of 1's followed by a single series of
0's. 
> Anything else is an invalid IPv6 mask.  For IPv4 I see hex and dotted quad
> equally.  Since this will only happen with a non-CIDR mask, which is the very
> rare case, after all, I think the previous IP(mask).String() was probably just
> fine.

Done.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode371
src/pkg/net/ip.go:371: bb.WriteString("0x")
On 2011/08/25 16:39:44, borman wrote:
> move this above the for and remove the check

Done.

http://codereview.appspot.com/4749043/diff/74001/src/pkg/net/ip.go#newcode412
src/pkg/net/ip.go:412: return s + ", " + n.Mask.String()
On 2011/08/25 16:45:51, borman wrote:
> On 2011/08/25 16:39:44, borman wrote:
> > This doesn't make sense to me.  It only is used when the mask is not a CIDR
> mask
> > but that is the time it can be most difficult to determine if something is a
> > mask or not "a.b.c.d, e.f.g.h" looks like 2 IP addresses while
> "a.b.c.d/e.f.g.h"
> > is more clear that the latter is a mask and not a second IP address.
> 
> Shouldn't this just be:
> 
> return n.IP.String() + "/" + n.Mask.String()

Done.

[mikio, 2011-08-25 21:54:06]

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

Please take another look.

[mikio, 2011-08-25 21:58:02]

On Fri, Aug 26, 2011 at 6:19 AM, Russ Cox <rsc@golang.org> wrote:

> I think we are converging, but this CL has grown too large.
> Let's regroup a bit.

ack.

[mikio, 2011-08-30 00:44:29]

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

Please take another look.

[rsc, 2011-08-31 22:08:05]

I'm happy with the API.  Splitting it up helped a ton.
I think Contains is not quite right.  The tests below
should tease it out.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode25
src/pkg/net/ip.go:25: // or 16-byte (IPv6) arrays as input.  Unless otherwise
Please delete this sentence (Unless otherwise...).
It's not true for things like IP.Mask and I don't
want people depending on this.

http://codereview.appspot.com/4749043/diff/88001/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/88001/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:172: }
add
if ip4 := tt.ip.To4(); ip4 != nil {
    if ok := tt.net.Contains(ip4); ok != tt.ok {
        t.Errorf("IPNet(%v).Contains(%v.To4()) = %v, want %v", tt.net, tt.ip,
ok, tt.ok)
    }
}

[mikio, 2011-09-01 06:20:12]

> I think Contains is not quite right.  The tests below
> should tease it out.

Am I missing something wrong? probably, but I don't see 
any problems on Contains method. Please let me know 
any good test cases for shooting the bug you found.

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/53005/src/pkg/net/ip.go#newcode25
src/pkg/net/ip.go:25: // or 16-byte (IPv6) arrays as input.  Unless otherwise
On 2011/08/31 22:08:05, rsc wrote:
> Please delete this sentence (Unless otherwise...).
> It's not true for things like IP.Mask and I don't
> want people depending on this.

Done.

http://codereview.appspot.com/4749043/diff/88001/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/88001/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:172: }
On 2011/08/31 22:08:05, rsc wrote:
> add
> if ip4 := tt.ip.To4(); ip4 != nil {
>     if ok := tt.net.Contains(ip4); ok != tt.ok {
>         t.Errorf("IPNet(%v).Contains(%v.To4()) = %v, want %v", http://tt.net,
tt.ip,
> ok, tt.ok)
>     }
> }

Done.

[mikio, 2011-09-01 06:20:37]

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

Please take another look.

[borman, 2011-09-01 15:31:11]

http://codereview.appspot.com/4749043/diff/88004/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/88004/src/pkg/net/ip.go#newcode409
src/pkg/net/ip.go:409: return n.IP.String() + "/" + itod(uint(l))
This will work for IPNet's produced by ParseCIDR but it will not work for an
IPNet made up of an IPv4 address that uses a 16 byte mask.  You will get a value
greater than 96 to the right of the /.

[mikio, 2011-09-02 05:21:19]

http://codereview.appspot.com/4749043/diff/88004/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/88004/src/pkg/net/ip.go#newcode409
src/pkg/net/ip.go:409: return n.IP.String() + "/" + itod(uint(l))
On 2011/09/01 15:31:11, borman wrote:
> This will work for IPNet's produced by ParseCIDR but it will not work for an
> IPNet made up of an IPv4 address that uses a 16 byte mask.  You will get a
value
> greater than 96 to the right of the /.

Done.

[mikio, 2011-09-02 05:21:51]

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

Please take another look.

[borman, 2011-09-02 05:38:40]

It is looking pretty good, but the mixture of 16 and 4 byte representations of
IPv4 addresses along with the now ability to mix IPv6 and IPv4 IP/Masks in IPNet
keeps adding special cases.  I guess the grand-unification theory is
complicated.

http://codereview.appspot.com/4749043/diff/90006/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/90006/src/pkg/net/ip.go#newcode411
src/pkg/net/ip.go:411: }
Okay, that looks like it should work.  The other case that is missing is when
n.IP is 16 bytes but n.Mask is 4 bytes.  If n.IP starts with the v4InV6Prefix
then it should be okay, but if someone has really mixed an IPv6 address with an
IPv4 mask...  I can't see how it would be valid so perhaps it should return
"<nil>" in that case (though "<nil>" is not quite right since it isn't empty).

[mikio, 2011-09-02 09:06:05]

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

Please take another look.

[mikio, 2011-09-02 09:10:17]

On Fri, Sep 2, 2011 at 2:38 PM,  <borman@google.com> wrote:

> It is looking pretty good, but the mixture of 16 and 4 byte
> representations of IPv4 addresses along with the now ability to mix IPv6
> and IPv4 IP/Masks in IPNet keeps adding special cases.

For now I'd like to follow the convention of existing routing stuff
implementation that IP mask is the mask, it belongs to an IP address.
E.g., func (m *RouteMessage) sockaddr() []Sockaddr in syscall/route_bsd.go

> I guess the grand-unification theory is complicated.

I agree with you but this case wont be a dragon... perhaps.... I hope so.

> http://codereview.appspot.com/4749043/diff/90006/src/pkg/net/ip.go#newcode411
> src/pkg/net/ip.go:411: }
> Okay, that looks like it should work.  The other case that is missing is
> when n.IP is 16 bytes but n.Mask is 4 bytes.  If n.IP starts with the
> v4InV6Prefix then it should be okay, but if someone has really mixed an
> IPv6 address with an IPv4 mask...  I can't see how it would be valid so
> perhaps it should return "<nil>" in that case (though "<nil>" is not
> quite right since it isn't empty).

Let me try it, PTAL.
And I think "<nil>" is okay, it's better that "" for debugging.

-- Mikio

[borman, 2011-09-02 16:49:57]

http://codereview.appspot.com/4749043/diff/88005/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/88005/src/pkg/net/ip.go#newcode371
src/pkg/net/ip.go:371: func networkNumberAndMask(n *IPNet) (IP, IPMask) {
I had several comments on edge cases and simplification.  To make sure I was
right I ended up just writing networkNumberAndMask and corresponding tests. 
PTAL.

func networkNumberAndMask(n *IPNet) (ip IP, m IPMask) {
        if ip = n.IP.To4(); ip == nil {
                ip = n.IP
                if len(ip) != IPv6len {
                        return nil, nil
                }
        }
        m = n.Mask
        switch len(m) {
        case IPv4len:
                if len(ip) != IPv4len {
                        return nil, nil
                }
        case IPv6len:
                if len(ip) == IPv4len {
                        m = m[12:]
                }
        default:
                return nil, nil
        }
        return
}

The test I used to test it was:

var (
	v4addr    = IP{1, 2, 3, 4}
	v4in6addr = IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 1, 2, 3, 4}
	v6addr    = IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}
	v4mask    = IPMask{255, 255, 255, 0}
	v4in6mask = IPMask{255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
255, 255, 255, 0}
	v6mask    = IPMask{255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}
	badaddr   = IP{1, 2, 3}
	badmask   = IPMask{255, 255, 0}
	z4mask    = IPMask{0, 0, 0, 0}
)

var networkNumberAndMaskTest = []struct {
	in  IPNet
	out IPNet
}{
	{
		in:  IPNet{v4addr, v4mask},
		out: IPNet{v4addr, v4mask},
	},
	{
		in:  IPNet{v4addr, v4in6mask},
		out: IPNet{v4addr, v4mask},
	},
	{
		in:  IPNet{v4in6addr, v4in6mask},
		out: IPNet{v4addr, v4mask},
	},
	{
		in:  IPNet{v4in6addr, v6mask},
		out: IPNet{v4addr, z4mask},
	},
	{
		in:  IPNet{v4addr, v6mask},
		out: IPNet{v4addr, z4mask},
	},
	{
		in:  IPNet{v6addr, v6mask},
		out: IPNet{v6addr, v6mask},
	},
	{
		in:  IPNet{v6addr, v4in6mask},
		out: IPNet{v6addr, v4in6mask},
	},
	{in: IPNet{v6addr, v4mask}},
	{in: IPNet{v4addr, badmask}},
	{in: IPNet{v4in6addr, badmask}},
	{in: IPNet{v6addr, badmask}},
	{in: IPNet{badaddr, v4mask}},
	{in: IPNet{badaddr, v4in6mask}},
	{in: IPNet{badaddr, v6mask}},
	{in: IPNet{badaddr, badmask}},
}

func TestNandM(t *testing.T) {
	for i, tt := range networkNumberAndMaskTest {
		ip, m := networkNumberAndMask(&tt.in)
		out := &IPNet{ip, m}
		if !reflect.DeepEqual(&tt.out, out) {
			t.Errorf("#%d: got %#v, want %#v", i, out, &tt.out)
		}
	}
}

http://codereview.appspot.com/4749043/diff/88005/src/pkg/net/ip.go#newcode405
src/pkg/net/ip.go:405: if l != len(nn) || l != len(m) {
just if l != len(nn) (we know len(nn) == len(m))

[mikio, 2011-09-03 03:10:14]

http://codereview.appspot.com/4749043/diff/88005/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/88005/src/pkg/net/ip.go#newcode371
src/pkg/net/ip.go:371: func networkNumberAndMask(n *IPNet) (IP, IPMask) {
On 2011/09/02 16:49:58, borman wrote:
> I had several comments on edge cases and simplification.  To make sure I was
> right I ended up just writing networkNumberAndMask and corresponding tests. 
> PTAL.

Looks concise and natural, thanks.

http://codereview.appspot.com/4749043/diff/88005/src/pkg/net/ip.go#newcode405
src/pkg/net/ip.go:405: if l != len(nn) || l != len(m) {
On 2011/09/02 16:49:58, borman wrote:
> just if l != len(nn) (we know len(nn) == len(m))

Done.

[mikio, 2011-09-03 03:10:33]

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

Please take another look.

[borman, 2011-09-03 03:40:39]

I am going to need more time to review the tests (I am in an airplane right now)
as they have very long constants.  I have to think they would be easier if we
had something like:

// NewIPPrefix returns an IPMask of size bits with the first n bits set to to 1.
// nil is returned if the Mask is impossible.
func NewIPPrefix(n, size int) IPMask

[mikio, 2011-09-04 07:31:09]

On 2011/09/03 03:40:39, borman wrote:

> I am going to need more time to review the tests (I am in an airplane right
now)
> as they have very long constants.

Much appreciate it.

> I have to think they would be easier if we
> had something like:
(snip)
> func NewIPPrefix(n, size int) IPMask

Or more straightforwardly: func CIDRMask(ones, bits int) IPMask.

[mikio, 2011-09-06 12:26:20]

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

Please take another look.

[borman, 2011-09-06 17:41:54]

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go#newcode70
src/pkg/net/ip.go:70: func CIDRMask(ones, bits int) IPMask {
I think this is right, but it is unfortunate that you cannot pass IPv4len or
IPv6len as a value.  I would like to hear what Russ thinks in this case.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go#newcode71
src/pkg/net/ip.go:71: if bits%8 != 0 {
bits <= 0 ||

or better yet

if bits != IPv4len * 8 && bits != IPv6len * 8

Then you don't have check the size of bits below.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go#newcode84
src/pkg/net/ip.go:84: m = make(IPMask, IPv6len)
You can do both the IPv4 and the IPv6 (actually, any size) case with the code
you have for IPv6, just use bits/8 for IPv6len.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:119: {"0.0.0.0/0", IPv4(0, 0, 0, 0), &IPNet{IPv4(0, 0, 0,
0), CIDRMask(0, 32)}, 0, nil},
I wish all the test cases had not been changed.  It is making it more
challenging to determine if we are covering the same tests as before.  As it
stands, I think the first two cases are missing.  I think we should be using the
same tests as were initially provided.  No need for the pfxlen argument, that is
not being tested here.

(I have been comparing against the original tests that existed before this CL)

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:253: 
Please add the tests for networkNumberAndMask.  If networkNumberAndMask fails
then both String and Contains can fail.  It is best not to hide the edge cases
of networkNumberAndMask in the tests for String and Contains.  This makes it
easier to track down bugs that may be introduced at a later time.

[mikio, 2011-09-07 06:04:05]

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go#newcode70
src/pkg/net/ip.go:70: func CIDRMask(ones, bits int) IPMask {
On 2011/09/06 17:41:54, borman wrote:
> I think this is right, but it is unfortunate that you cannot pass IPv4len or
> IPv6len as a value.  I would like to hear what Russ thinks in this case.

agreed.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go#newcode71
src/pkg/net/ip.go:71: if bits%8 != 0 {
On 2011/09/06 17:41:54, borman wrote:
> bits <= 0 ||
> 
> or better yet
> 
> if bits != IPv4len * 8 && bits != IPv6len * 8
> 
> Then you don't have check the size of bits below.

Done.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip.go#newcode84
src/pkg/net/ip.go:84: m = make(IPMask, IPv6len)
On 2011/09/06 17:41:54, borman wrote:
> You can do both the IPv4 and the IPv6 (actually, any size) case with the code
> you have for IPv6, just use bits/8 for IPv6len.

Done.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:119: {"0.0.0.0/0", IPv4(0, 0, 0, 0), &IPNet{IPv4(0, 0, 0,
0), CIDRMask(0, 32)}, 0, nil},
On 2011/09/06 17:41:54, borman wrote:
> I wish all the test cases had not been changed.  It is making it more
> challenging to determine if we are covering the same tests as before.  As it
> stands, I think the first two cases are missing.  I think we should be using
the
> same tests as were initially provided.  No need for the pfxlen argument, that
is
> not being tested here.
> 
> (I have been comparing against the original tests that existed before this CL)

Done.

http://codereview.appspot.com/4749043/diff/91007/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:253: 
On 2011/09/06 17:41:54, borman wrote:
> Please add the tests for networkNumberAndMask.  If networkNumberAndMask fails
> then both String and Contains can fail.  It is best not to hide the edge cases
> of networkNumberAndMask in the tests for String and Contains.  This makes it
> easier to track down bugs that may be introduced at a later time.

Done.

[mikio, 2011-09-07 06:04:24]

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

Please take another look.

[borman, 2011-09-07 06:41:24]

LGTM with the addition of the missing test case.

http://codereview.appspot.com/4749043/diff/91008/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/91008/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:133: {"2001:DB8::/48", ParseIP("2001:DB8::"),
&IPNet{ParseIP("2001:DB8::"), IPMask(ParseIP("ffff:ffff:ffff::"))}, nil},
This looks good.  Just one more test, say 2001:DB8::1/48 to demonstrate that
ParseCIDR works works for IPv6 a well as IPv4 when the IP address has the host
part as well as the network part.

[mikio, 2011-09-07 07:08:57]

http://codereview.appspot.com/4749043/diff/91008/src/pkg/net/ip_test.go
File src/pkg/net/ip_test.go (right):

http://codereview.appspot.com/4749043/diff/91008/src/pkg/net/ip_test.go#newco...
src/pkg/net/ip_test.go:133: {"2001:DB8::/48", ParseIP("2001:DB8::"),
&IPNet{ParseIP("2001:DB8::"), IPMask(ParseIP("ffff:ffff:ffff::"))}, nil},
On 2011/09/07 06:41:24, borman wrote:
> This looks good.  Just one more test, say 2001:DB8::1/48 to demonstrate that
> ParseCIDR works works for IPv6 a well as IPv4 when the IP address has the host
> part as well as the network part.

Done.

[mikio, 2011-09-07 07:09:13]

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

Please take another look.

[borman, 2011-09-07 16:10:22]

LGTM

[rsc, 2011-09-07 18:00:44]

LGTM

we made it to the end!

[rsc, 2011-09-07 18:01:16]

*** Submitted as http://code.google.com/p/go/source/detail?r=ee93036cddf9 ***

net: ParseCIDR returns IPNet instead of IPMask

Note that this CL will break your existing code which uses
ParseCIDR.

This CL changes ParseCIDR("172.16.253.121/28") to return
the IP address "172.16.253.121", the network implied by the
network number "172.16.253.112" and mask "255.255.255.240".

R=rsc, borman
CC=golang-dev
http://codereview.appspot.com/4749043

Committer: Russ Cox <rsc@golang.org>

http://codereview.appspot.com/4749043/diff/81002/src/pkg/net/ip.go
File src/pkg/net/ip.go (right):

http://codereview.appspot.com/4749043/diff/81002/src/pkg/net/ip.go#newcode68
src/pkg/net/ip.go:68: // CIDRMask returns an IPMask of size bits with the first
n bits
move 'set to 1' up here.
next line:
// It returns nil if the parameters are invalid.

http://codereview.appspot.com/4749043/diff/81002/src/pkg/net/ip.go#newcode70
src/pkg/net/ip.go:70: func CIDRMask(ones, bits int) IPMask {
s/ones, bits/n, size/ to match documentation.

[rsc, 2011-09-07 18:36:30]

Forgot I had written these comments.
I will send a CL.

On Wed, Sep 7, 2011 at 14:01,  <rsc@golang.org> wrote:
> http://codereview.appspot.com/4749043/diff/81002/src/pkg/net/ip.go
> File src/pkg/net/ip.go (right):
>
> http://codereview.appspot.com/4749043/diff/81002/src/pkg/net/ip.go#newcode68
> src/pkg/net/ip.go:68: // CIDRMask returns an IPMask of size bits with
> the first n bits
> move 'set to 1' up here.
> next line:
> // It returns nil if the parameters are invalid.
>
> http://codereview.appspot.com/4749043/diff/81002/src/pkg/net/ip.go#newcode70
> src/pkg/net/ip.go:70: func CIDRMask(ones, bits int) IPMask {
> s/ones, bits/n, size/ to match documentation.

[mikio, 2011-09-08 02:54:12]

On Thu, Sep 8, 2011 at 3:00 AM, Russ Cox <rsc@golang.org> wrote:

> we made it to the end!

Yeah, many thanks to both of you, Paul and Russ.