Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python3 port #3

Closed
wants to merge 10 commits into from
6 changes: 3 additions & 3 deletions pox/forwarding/l3_learning.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def _handle_openflow_PacketIn (self, event):
# Ignore LLDP packets
return

if isinstance(packet.__next__, ipv4):
if isinstance(packet.next, ipv4):
log.debug("%i %i IP %s => %s", dpid,inport,
packet.next.srcip,packet.next.dstip)

Expand Down Expand Up @@ -269,8 +269,8 @@ def _handle_openflow_PacketIn (self, event):
msg.in_port = inport
event.connection.send(msg)

elif isinstance(packet.__next__, arp):
a = packet.__next__
elif isinstance(packet.next, arp):
a = packet.next
log.debug("%i %i ARP %s %s => %s", dpid, inport,
{arp.REQUEST:"request",arp.REPLY:"reply"}.get(a.opcode,
'op:%i' % (a.opcode,)), a.protosrc, a.protodst)
Expand Down
2 changes: 1 addition & 1 deletion pox/host_tracker/host_tracker.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ def _handle_openflow_PacketIn (self, event):

macEntry.refresh()

(pckt_srcip, hasARP) = self.getSrcIPandARP(packet.__next__)
(pckt_srcip, hasARP) = self.getSrcIPandARP(packet.next)
if pckt_srcip is not None:
self.updateIPInfo(pckt_srcip,macEntry,hasARP)

Expand Down
4 changes: 2 additions & 2 deletions pox/info/packet_dump.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def _handle_PacketIn (event):
break
return
if not hasattr(p, 'next'): break
p = p.__next__
p = p.next

if not show: return

Expand All @@ -62,7 +62,7 @@ def _handle_PacketIn (event):
msg += "[%s bytes]" % (len(p),)
break
msg += "[%s]" % (p.__class__.__name__,)
p = p.__next__
p = p.next

if _max_length:
if len(msg) > _max_length:
Expand Down
78 changes: 65 additions & 13 deletions pox/lib/addresses.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

@larabr larabr Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated all constructors to store values as bytes, in fact this is probably an outdated commit... ord is not needed anymore

return name + separator + rest

return separator.join(('%02x' % (ord(x),) for x in self._value))
Expand Down Expand Up @@ -387,14 +387,41 @@ def multicast_ethernet_address (self):
def __str__ (self):
return self.toStr()

def __cmp__ (self, other):
if other is None: return 1
def __eq__(self, other):
try:
if not isinstance(other, IPAddr):
other = IPAddr(other)
return cmp(self.toUnsigned(), other.toUnsigned())
return self.toUnsigned() == other.toUnsigned()
except:
return -other.__cmp__(self)
return other.__eq__(self)

def __ne__(self, other):
try:
if not isinstance(other, IPAddr):
other = IPAddr(other)
return self.toUnsigned() != other.toUnsigned()
except:
return other.__ne__(self)

def __lt__(self, other):
try:
if not isinstance(other, IPAddr):
other = IPAddr(other)
return self.toUnsigned() < other.toUnsigned()
except:
# reversed order
return other.__gt__(self)

def __gt__(self, other):
try:
if not isinstance(other, IPAddr):
other = IPAddr(other)
return self.toUnsigned() > other.toUnsigned()
except:
# reversed order
return other.__lt__(self)


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the functools.total_ordering decorator here instead of a bunch of manual implementations?

Copy link
Author

@larabr larabr Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def __hash__ (self):
return self._value.__hash__()
Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

num >>= 8
return cls.from_raw(o)

Expand Down Expand Up @@ -555,7 +582,7 @@ def to_ipv4 (self, check_ipv4 = True):
def num (self):
o = 0
for b in self._value:
o = (o << 8) | ord(b)
o = (o << 8) | b
return o

@property
Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Author

@larabr larabr Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is always bytes now, so string conversion with ord is not needed anymore

(self._value[i:i+2] for i in range(0,16,2))]

if (ipv4 is None and self.is_ipv4_mapped) or ipv4:
Expand Down Expand Up @@ -737,14 +764,39 @@ def fmt (n):
def __str__ (self):
return self.to_str()

def __cmp__ (self, other):
if other is None: return 1
def __eq__(self, other):
try:
if not isinstance(other, type(self)):
other = type(self)(other)
return self._value == other._value
except:
return other.__eq__(self)

def __ne__(self, other):
try:
if not isinstance(other, type(self)):
other = type(self)(other)
return not self._value == other._value
except:
return other.__ne__(self)

def __lt__(self, other):
try:
if not isinstance(other, type(self)):
other = type(self)(other)
return self._value < other._value
except:
# reversed order
return other.__gt__(self)

def __gt__(self, other):
try:
if not isinstance(other, type(self)):
other = type(self)(other)
return cmp(self._value, other._value)
return self._value > other._value
except:
return -cmp(other,self)
# reversed order
return other.__lt__(self)

def __hash__ (self):
return self._value.__hash__()
Expand All @@ -764,7 +816,7 @@ def set_mac (self, eth):
e = list(EthAddr(eth).toTuple())
e[0] ^= 2
e[3:3] = [0xff,0xfe]
e = ''.join(chr(b) for b in e)
e = b''.join(chr(b).encode('latin-1') for b in e)
return IPAddr6.from_raw(self._value[:8]+e)


Expand Down
8 changes: 4 additions & 4 deletions pox/lib/packet/icmp.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@
# stringizing.
# (Note: There may actually be a better way now using _to_str().)
def _str_rest (s, p):
if p.__next__ is None:
if p.next is None:
return s
if isinstance(p.__next__, str):
return "[%s bytes]" % (len(p.__next__),)
return s+str(p.__next__)
if isinstance(p.next, str):
return "[%s bytes]" % (len(p.next),)
return s+str(p.next)


#----------------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion pox/lib/packet/ipv4.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def parse(self, raw):
else:
self.next = raw[self.hl*4:length]

if isinstance(self.__next__, packet_base) and not self.next.parsed:
if isinstance(self.next, packet_base) and not self.next.parsed:
self.next = raw[self.hl*4:length]

def checksum(self):
Expand Down
2 changes: 1 addition & 1 deletion pox/lib/packet/ipv6.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ def parse (self, raw, offset=0):
else:
self.next = raw[offset:offset+length]

if isinstance(self.__next__, packet_base) and not self.next.parsed:
if isinstance(self.next, packet_base) and not self.next.parsed:
self.next = raw[offset:offset+length]

def add_header (self, eh):
Expand Down
16 changes: 8 additions & 8 deletions pox/lib/packet/packet_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def __str__(self):
lg.debug("str(%s): %s" % (self.__class__.__name__, e))
return "[%s:Bad representation]" % (self.__class__.__name__,)
return "[%s l:%i%s]" % (self.__class__.__name__, len(self),
"" if self.__next__ else " *")
"" if self.next else " *")

def dump(self):
p = self
Expand All @@ -129,7 +129,7 @@ def dump(self):
m.append("[%s]" % (p.__class__.__name__,))
break
m.append(str(p))
p = p.__next__
p = p.next
return "".join(m)

def find(self, proto):
Expand All @@ -141,7 +141,7 @@ def find(self, proto):
if self.__class__.__name__ == proto and self.parsed:
return self
else:
if self.__next__ and isinstance(self.__next__, packet_base):
if self.next and isinstance(self.next, packet_base):
return self.next.find(proto)
else:
return None
Expand All @@ -155,7 +155,7 @@ def payload (self):
setting the new payload's "prev" field to point back to its new
container (the same as the set_payload() method).
"""
return self.__next__
return self.next

@payload.setter
def payload (self, new_payload):
Expand Down Expand Up @@ -192,16 +192,16 @@ def unpack (cls, raw, prev=None):
def pack(self):
'''Convert header and payload to bytes'''

if self.parsed is False and self.raw is not None and self.__next__ is None:
if self.parsed is False and self.raw is not None and self.next is None:
return self.raw

self.pre_hdr()

if self.__next__ == None:
if self.next == None:
return self.hdr(b'')
elif isinstance(self.__next__, packet_base):
elif isinstance(self.next, packet_base):
rest = self.next.pack()
else:
rest = self.__next__
rest = self.next

return self.hdr(rest) + rest
6 changes: 3 additions & 3 deletions pox/lib/packet/tcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -703,12 +703,12 @@ def checksum (self, unparsed=False, payload=None):
else:
if payload is not None:
pass
elif isinstance(self.__next__, packet_base):
elif isinstance(self.next, packet_base):
payload = self.next.pack()
elif self.__next__ is None:
elif self.next is None:
payload = bytes()
else:
payload = self.__next__
payload = self.next
payload = self.hdr(None, calc_checksum = False) + payload
payload_len = len(payload)

Expand Down
6 changes: 3 additions & 3 deletions pox/lib/packet/udp.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,12 @@ def checksum(self, unparsed=False):
payload_len = len(self.raw)
payload = self.raw
else:
if isinstance(self.__next__, packet_base):
if isinstance(self.next, packet_base):
payload = self.next.pack()
elif self.__next__ is None:
elif self.next is None:
payload = bytes()
else:
payload = self.__next__
payload = self.next
payload_len = udp.MIN_LEN + len(payload)

myhdr = struct.pack('!HHHH', self.srcport, self.dstport,
Expand Down
4 changes: 2 additions & 2 deletions pox/lib/pxpcap/dump_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def cb (data, parser):
break
return
if not hasattr(p, 'next'): break
p = p.__next__
p = p.next

if not show: return

Expand All @@ -72,7 +72,7 @@ def cb (data, parser):
msg += "[%s bytes]" % (len(p),)
break
msg += "[%s]" % (p.__class__.__name__,)
p = p.__next__
p = p.next

if _max_length:
if len(msg) > _max_length:
Expand Down
8 changes: 4 additions & 4 deletions pox/openflow/libopenflow_01.py
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,7 @@ def from_packet (cls, packet, in_port = None, spec_frags = False):
match.dl_src = packet.src
match.dl_dst = packet.dst
match.dl_type = packet.type
p = packet.__next__
p = packet.next

# Is this in the spec?
if packet.type < 1536:
Expand All @@ -971,12 +971,12 @@ def from_packet (cls, packet, in_port = None, spec_frags = False):
if isinstance(p, llc):
if p.has_snap and p.oui == '\0\0\0':
match.dl_type = p.eth_type
p = p.__next__
p = p.next
if isinstance(p, vlan):
match.dl_type = p.eth_type
match.dl_vlan = p.id
match.dl_vlan_pcp = p.pcp
p = p.__next__
p = p.next
else:
match.dl_vlan = OFP_VLAN_NONE
match.dl_vlan_pcp = 0
Expand All @@ -991,7 +991,7 @@ def from_packet (cls, packet, in_port = None, spec_frags = False):
match.tp_src = 0
match.tp_dst = 0
return match
p = p.__next__
p = p.next

if isinstance(p, udp) or isinstance(p, tcp):
match.tp_src = p.srcport
Expand Down
5 changes: 2 additions & 3 deletions tests/unit/lib/addresses_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def test_bad_cidr_succeed (self):

def test_byte_order (self):
self.assertEqual(IPAddr(IPAddr('1.2.3.4').toSigned()).raw,
'\x01\x02\x03\x04')
b'\x01\x02\x03\x04')

#TODO: Clean up these IPv6 tests
class IPv6Tests (unittest.TestCase):
Expand All @@ -89,15 +89,14 @@ def test_part2 (self):
"""
Basic IPv6 address tests (part 2)
"""
h = '\xfe\x80\x00\x00\x00\x00\x00\x00\xba\x8d\x12\xff\xfe\x2a\xdd\x6e'
h = b'\xfe\x80\x00\x00\x00\x00\x00\x00\xba\x8d\x12\xff\xfe\x2a\xdd\x6e'
a = IPAddr6.from_raw(h)
assert str(a) == 'fe80::ba8d:12ff:fe2a:dd6e'
assert a.raw == h

assert a.num == 0xfe80000000000000ba8d12fffe2add6e
assert IPAddr6.from_num(a.num) == a

assert a.is_multicast is False
assert IPAddr6("FF02:0:0:0:0:0:0:1").is_multicast

assert IPAddr6('2001:db8:1:2::').set_mac('00:1D:BA:06:37:64') \
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/openflow/topology_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class MockSwitch(EventMixin):
def __init__(self):
EventMixin.__init__(self)
self.connected = True
self._xid_generator = itertools.count(1).__next__
self._xid_generator = itertools.count(1).next
self.sent = []

def send(self, msg):
Expand Down
2 changes: 1 addition & 1 deletion tools/pox-pydoc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1922,7 +1922,7 @@ def __init__(self, roots, children, descendp):
self.children = children
self.descendp = descendp

def __next__(self):
def next(self):
if not self.state:
if not self.roots:
return None
Expand Down