code review 4950046: net: change the internal form of IPMask for IPv4

net: change the internal form of IPMask for IPv4

This CL changes the internal form of IPMask for IPv4
from 16-byte to 4-byte, also adds Size method to IPMask
struct and changes output string format of IPMask.String
method.

[mikio, 2011-08-26 00:47:16]

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

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

[mikio, 2011-08-26 01:56:39]

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

Please take another look.

[mikio, 2011-08-26 02:24:22]

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

Please take another look.

[rsc, 2011-08-26 19:55:40]

Close.

http://codereview.appspot.com/4950046/diff/11001/src/pkg/net/ip.go
File src/pkg/net/ip.go (left):

http://codereview.appspot.com/4950046/diff/11001/src/pkg/net/ip.go#oldcode217
src/pkg/net/ip.go:217: if len(mask) == IPv6len && len(ip) == IPv4len &&
allFF(mask[:12]) {
Please add this back.
This is still a valid operation.  You can do, say,

IPv4(1,2,3,4).Mask(IPMask(ParseIP("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ff00")))

Please add a test for this case so that we stop deleting it.

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

http://codereview.appspot.com/4950046/diff/11001/src/pkg/net/ip.go#newcode562
src/pkg/net/ip.go:562: ip = ip[12:]
You need a temporary here.  ParseCIDR should continue to return
a length-16 IP address, since that is what the package docs say
happens.

mip := ip
if iplen == IPv4len {
    mip = ip[12:]
}
for i := range mip {
    if mip[i]&^mask[i] != 0 {
...

[mikio, 2011-08-27 02:29:58]

http://codereview.appspot.com/4950046/diff/11001/src/pkg/net/ip.go
File src/pkg/net/ip.go (left):

http://codereview.appspot.com/4950046/diff/11001/src/pkg/net/ip.go#oldcode217
src/pkg/net/ip.go:217: if len(mask) == IPv6len && len(ip) == IPv4len &&
allFF(mask[:12]) {
On 2011/08/26 19:55:40, rsc wrote:
> Please add this back.
> This is still a valid operation.  You can do, say,
> 
> IPv4(1,2,3,4).Mask(IPMask(ParseIP("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ff00")))
> 
> Please add a test for this case so that we stop deleting it.
> 

Done.

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

http://codereview.appspot.com/4950046/diff/11001/src/pkg/net/ip.go#newcode562
src/pkg/net/ip.go:562: ip = ip[12:]
On 2011/08/26 19:55:40, rsc wrote:
> You need a temporary here.  ParseCIDR should continue to return
> a length-16 IP address, since that is what the package docs say
> happens.
> 
> mip := ip
> if iplen == IPv4len {
>     mip = ip[12:]
> }
> for i := range mip {
>     if mip[i]&^mask[i] != 0 {
> ...
> 
> 

Done.

[mikio, 2011-08-27 02:30:18]

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

Please take another look.

[mikio, 2011-08-27 04:55:10]

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

Please take another look.

[rsc, 2011-08-29 14:48:36]

LGTM

[rsc, 2011-08-29 14:49:07]

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

net: change the internal form of IPMask for IPv4

This CL changes the internal form of IPMask for IPv4
from 16-byte to 4-byte, also adds Size method to IPMask
struct and changes output string format of IPMask.String
method.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4950046

Committer: Russ Cox <rsc@golang.org>