Python3 port #3
Python3 port #3
Conversation
pox/lib/addresses.py
Outdated
| # reversed order | ||
| return other.__lt__(self) | ||
|
|
||
|
|
There was a problem hiding this comment.
Can we use the functools.total_ordering decorator here instead of a bunch of manual implementations?
There was a problem hiding this comment.
I guess so, but I believe this is a special case which I kept from the original code: we try to cast other to the same type as self, but if we fail, we do the reverse and call the compare function implemented in other.
I am not a fan of this logic, but I also do not know whether there were strong reasons to do it this way in the first place.
pox/lib/addresses.py
Outdated
| @@ -199,7 +199,7 @@ def to_str (self, separator = ':', resolve_names = False): | |||
| # Don't even bother for local (though it should never match and OUI!) | |||
| name = _eth_oui_to_name.get(self._value[:3]) | |||
| if name: | |||
| rest = separator.join('%02x' % (ord(x),) for x in self._value[3:]) | |||
| rest = separator.join('%02x' % (ord(x),) for x in self._value[3:]) #TODO ERROR | |||
There was a problem hiding this comment.
Offhand, this seems like it should be fine to me. I think the problem is probably that there are cases where _value doesn't end up as a bytes object and is a string instead, which it shouldn't be. This is probably because some of the code paths in the constructor were sloppy. (On that note, there are constructor code paths that can be improved since raw inputs and hex inputs are now more easily distinguished via their type.)
There was a problem hiding this comment.
I updated all constructors to store values as bytes, in fact this is probably an outdated commit... ord is not needed anymore
| @@ -436,7 +463,7 @@ def from_num (cls, num): | |||
| """ | |||
| o = b'' | |||
| for i in range(16): | |||
| o = chr(num & 0xff) + o | |||
| o = chr(num & 0xff).encode('latin-1') + o | |||
There was a problem hiding this comment.
I think we should probably just avoid chr(). I don't think there's a direct equivalent for bytes objects, but we could make a chrb or something and put it in util or something. Maybe along the lines of chrb = lambda x: struct.pack('B', x) ?
There was a problem hiding this comment.
This should work, but the notion of chr changes a bit in python3 (i.e. it's about unicode specifically) so I would call it something different.
| @@ -690,7 +717,7 @@ def to_str (self, zero_drop = True, section_drop = True, ipv4 = None): | |||
| by passing ipv4=True; this probably only makes sense if .is_ipv4_compatible | |||
| (or .is_ipv4_mapped, of course). | |||
| """ | |||
| o = [ord(lo) | (ord(hi)<<8) for hi,lo in | |||
| o = [lo | (hi<<8) for hi,lo in | |||
There was a problem hiding this comment.
I'm surprised this works. I wonder if we should do something like I suggested for chr() and have an ordb() or whatever like ordb = lambda x: struct.unpack('B', x)[0].
There was a problem hiding this comment.
The value is always bytes now, so string conversion with ord is not needed anymore
|
POX halosaur now no longer marks Python 3 as experimental! Thanks for your initial help getting this rolling! Even if I used some different techniques to fix things, your calling attention to places that needed fixing was very valuable! (The new Special Thanks section of the README acknowledges this.) |
Working to fix: noxrepo#192
Port done starting from 2to3 conversion, then fixing issues from there (mostly to do with bytes/string incompatibility in python3, which also affects the usage of
ord(),chr().All unit tests pass, plus I run
messenger/test_client.pywhich successfully received the welcome message.Since not all modules were covered by tests, the code likely needs more fixes. Besides what I tried above, do you have any suggestions on what to do to test further e.g. the packet modules?