Re: code review 4749043: net: ParseCIDR returns IPNet instead of IPMask (issue 4749043)

[mikioh...@gmail.com]

Hello r...@golang.org, bor...@google.com (cc:
golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/4749043/

[r...@golang.org]

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.

http://codereview.appspot.com/4749043/

[bor...@google.com]

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.

http://codereview.appspot.com/4749043/

[Russ Cox]

> 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 Hara]

On Tue, Aug 23, 2011 at 3:01 AM, <r...@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.

[Russ Cox]

>> 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

[Mikio Hara]

On Tue, Aug 23, 2011 at 3:26 AM, Russ Cox <r...@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?

[Russ Cox]

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 Hara]

On Tue, Aug 23, 2011 at 3:50 AM, Russ Cox <r...@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

[mikioh...@gmail.com]

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.

http://codereview.appspot.com/4749043/

[mikioh...@gmail.com]

[mikioh...@gmail.com]

[mikioh...@gmail.com]

[mikioh...@gmail.com]

[r...@golang.org]

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))

http://codereview.appspot.com/4749043/

[mikioh...@gmail.com]

> 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.

http://codereview.appspot.com/4749043/

[mikioh...@gmail.com]

[bor...@google.com]

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.

http://codereview.appspot.com/4749043/

[bor...@google.com]

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()

http://codereview.appspot.com/4749043/

[Russ Cox]

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

[mikioh...@gmail.com]

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.

http://codereview.appspot.com/4749043/

[mikioh...@gmail.com]

[Mikio Hara]

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

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

ack.