Re: ipaddr-py: New IPv4 netmask/hostmask parser. (issue 13745043)

[Peter Moody]

Sorry for the delay, looking at this now.

note that ipaddr is effectively deprecated in favor of ipaddress which is in the stdlib of python 3.2 (not that making this patch against ipaddr as opposed to ipaddress delayed its review).

On Tue, Sep 17 2013 at 11:46, pma...@google.com wrote:
> Reviewers: Peter Moody,
>
> Description:
> This operates at the bit level, instead of making strange assumptions at
> the octet level, and thus fixes an assortment of bugs.
>
> Note that this could be trivially extended to support IPv6 netmasks and
> hostmasks, but that's probably not useful because everyone uses CIDR
> notation.
>
> Added:
> - _prefix_from_prefix_string: Parse a number, with bounds checking.
> - _prefix_from_ip_string: Parse IPv4 netmask/hostmask string.
>
> Improved:
> - _prefix_from_ip_int: Only accept inputs with the bit sequence 1*0*.
>
> Removed:
> - _ip_string_from_prefix
> - _is_hostmask
> - _is_valid_netmask
>
>
> Please review this at https://codereview.appspot.com/13745043/
>
> Affected files (+141, -124 lines):
> M trunk/ipaddr.py
> M trunk/ipaddr_test.py

[pmo...@google.com]

Thanks much for doing this.

If it's not too much trouble, could you include some information on the
bugs in the existing parser?


https://codereview.appspot.com/13745043/

[pmo...@google.com]

https://codereview.appspot.com/13745043/diff/1/trunk/ipaddr.py
File trunk/ipaddr.py (right):

https://codereview.appspot.com/13745043/diff/1/trunk/ipaddr.py#newcode922
trunk/ipaddr.py:922: # Try matching a netmask (1*0*). Note that the two
ambiguous cases
(1*0*).

I'm having a hard time understanding what that's signifying. Is that a
regular expression?

https://codereview.appspot.com/13745043/diff/1/trunk/ipaddr_test.py
File trunk/ipaddr_test.py (right):

https://codereview.appspot.com/13745043/diff/1/trunk/ipaddr_test.py#newcode458
trunk/ipaddr_test.py:458:
I'm not sure I understand the purpose of the tests between 430-455. It
looks like a series of

addr1 = '0.0.0.0/%d'
addr2 = '0.0.0.0/%d'

assertEqual(addr1,addr2)

I presume I'm missing something more subtle?

https://codereview.appspot.com/13745043/diff/1/trunk/ipaddr_test.py#newcode461
trunk/ipaddr_test.py:461: # netmasks are rarely seen in the wild.
nit, I think it's all but expressly prohibited.

http://tools.ietf.org/html/rfc4291#section-2.3

https://codereview.appspot.com/13745043/

[pmo...@google.com]

lgtm

https://codereview.appspot.com/13745043/

[pmo...@google.com]

On 2013/09/21 16:32:52, Peter Moody wrote:
> lgtm

applied in 669c0368e3e4 .

thanks again!

https://codereview.appspot.com/13745043/