[#9449] IPv6 address changes in 3.7 #1018
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #1018 +/- ##
==========================================
- Coverage 91.65% 91.64% -0.02%
==========================================
Files 844 844
Lines 150582 150618 +36
Branches 13148 13154 +6
==========================================
+ Hits 138016 138033 +17
- Misses 10465 10481 +16
- Partials 2101 2104 +3 |
hawkowl
changed the title
There was a problem hiding this comment.
I'd really like it if the language in some of these comments were tightened up, and explicit references to the specific API change in 3.7 were added so it's a little clearer what information we're preserving and why. Scope IDs are a weird beast and it's good to be precise when talking about them - the scope ID in an IPv6 address is very much not quite the same thing as a physical network interface (although they do frequently correspond, and sometimes interface names are valid scope IDs).
Should the .misc be a non-.misc that says something about 3.7 compatibility, or is this something that's obscure enough that most people won't notice?
One thing I'm also curious about, though, is why you're so interested in preserving the scope ID within the address literal portion of the 4-tuple, given that the scope ID is already present as an integer at the end of that 4-tuple.
However, I'm willing to take it on faith that there's a good compatibility reason you needed to do this, and things break on 3.7 without it. The test coverage is sufficient and the logic looks correct. So if you could just please add this info, correct any "interface -> scopeID" language where appropriate, and then land that would be great.
src/twisted/conch/scripts/ckeygen.py
Outdated
| @@ -189,7 +189,7 @@ def changePassPhrase(options): | |||
| 'Enter file in which the key is (%s): ' % filename) | |||
| try: | |||
| key = keys.Key.fromFile(options['filename']) | |||
| except keys.EncryptedKeyError as e: | |||
| except keys.EncryptedKeyError: | |||
There was a problem hiding this comment.
This seems… extraneous, but I'll allow it :)
There was a problem hiding this comment.
failing txchecker because it hates me >:C
src/twisted/internet/tcp.py
Outdated
| """ | ||
| Return a 2-tuple of socket IP and port for IPv4 and a 4-tuple of | ||
| socket IP, port, flowInfo, and scopeID. With IPv6 addresses, this | ||
| preserves the interface portion. |
There was a problem hiding this comment.
Out of "socket IP, port, flowInfo, and scopeID", none of those things are obviously called "interface". Can you clarify just a little as to what is being preserved, and why this is important (or what wouldn't preserve it?) I think for future maintainers understanding that this is papering over a 3.6 / 3.7 difference is probably pretty important.
| @@ -580,16 +580,24 @@ def handleAccept(self, rc, evt): | |||
| rAddr = (nativeString(rAddr[0]), rAddr[1]) | |||
| assert family == self.addressFamily | |||
|
|
|||
| # Build a proper IPv6 address structure, if it exists | |||
There was a problem hiding this comment.
By "proper" I think you mean "including scope ID, if necessary"?
src/twisted/internet/tcp.py
Outdated
| @@ -429,7 +462,12 @@ def _setRealAddress(self, address): | |||
| and the 'host' portion will always be an IP address, not a DNS | |||
| name. | |||
| """ | |||
| self.realAddress = address | |||
| if len(address) == 4: | |||
| # IPv6, make sure we have the interface portion | |||
src/twisted/internet/tcp.py
Outdated
| """ | ||
| if len(addr) == 4: | ||
| # IPv6 | ||
| host = socket.getnameinfo(addr, socket.NI_NUMERICHOST)[0] |
There was a problem hiding this comment.
I think you really want socket.NI_NUMERICHOST | socket.NI_NUMERICSERV; you don't need to do service-name lookup, so this is asking the OS to do some pointless I/O on a part of the address you'll immediately throw away.
https://tm.tl/9449