Repository navigation
fix: IP() refuses integers out of range - #30
Conversation
marcos-mendez
left a comment
There was a problem hiding this comment.
Thanks @Nandika-Gupta, this is a clean follow-up.
I ran it: 956 passed, ipaddr.py stays at 100%. The range check is right (0 to 232-1, both ends tested), and the error matches the existing "illegal ip" style. Good catch on IPRange.broadcast: with the new check, computing network + 2**32 - netmask - 1 through IP's operators would overshoot 232 in the middle of the expression and raise, so doing it on plain ints is exactly the right fix. The test that 255.255.255.255 + 1 now raises is a nice touch too.
A correction to something I wrote on #29: I said the network screens use IPRange to validate host addresses. I checked, and they don't: confconsole.py only uses ipaddr.is_legal_ip, and the final network checks use Python's stdlib ipaddress. So IP and IPRange have no runtime callers today, and this change can't break a screen. The strict < in __contains__ is still worth documenting as you did, but my reason for it was wrong. Sorry about that.
Approving; I'll merge.
|
Oh got it, thanks for clarifying! Glad the fix works. |
Follow-up from #29, as suggested there.
IP() took any integer, so IP(2**32) or IP(-1) was created fine and only failed later in str() with a struct.error. It now raises Error("ip out of range") at construction, like the "illegal ip" error for bad strings.
IPRange built its broadcast address as
network + 2**32 - netmask - 1, which goes past 2**32 for a moment with IP's operators, so that line now does the maths on plain ints. Same result as before.Tests: 0 and 232-1 still work, -1 and 232 raise, and 255.255.255.255 + 1 raises. Full suite: 956 passed, ipaddr.py stays at 100%. Added a changelog entry (2.2.3+keel16).