int/long EthAddr type?

[MurphyMc]

08ea0753ff883 adds int/long reading to EthAddr's init. I had specifically omitted this because I think it doesn't make much sense and I want to discourage it. My feeling is pretty much that an EthAddr should NEVER be in an integer type. IP addresses are bad enough and at least they're a reasonable size. Do we have a use case for this?

8bc63c95e4b61 alters how the integer is interpreted (not too surprising since, in my opinion, there is no good way to interpret how an ethernet address fits into an integer type). (And it incorrectly references me as having done this in the first place!)

If we do have a use case for this functionality, the code from 8bc63c95e4b61 could be improved. But if we have a use case for this functionality, I may need to be convinced that the fact that we have such a use case doesn't indicate that the code using it has a bug. :)

[colin-scott]

Yes, we have a use-case for this. HSA stores ethernet addresses as Ints, and we need a way of translating between HSA's representation and ours.

In sdn-debugger:

$ git grep EthAddr

headerspace/config_parser/openflow_parser.py: #print "Eth String is:" + str(EthAddr(bytes_to_int(bytes))) headerspace/config_parser/openflow_parser.py: return str(EthAddr(bytes_to_int(bytes))) headerspace/config_parser/openflow_parser.py: addr = EthAddr(addr).toInt() headerspace/config_parser/openflow_parser.py: value = "EthAddr(\"%s\")" % value headerspace/config_parser/openflow_parser.py: # Odd that ofp_match takes EthAddrs, but just strings for IpAddrs... sts/topology_generator.py:from pox.lib.addresses import EthAddr, IPAddr sts/topology_generator.py: hw_addr=EthAddr("00:00:00:00:%02x:%02x" % (switch_id, port_no)) ) sts/topology_generator.py: mac = EthAddr("12:34:56:78:%02x:%02x" % (switch.dpid, port.port_no)) sts/topology_generator.py: portland_mac = EthAddr("00:00:00:%02x:%02x:%02x" % (current_pod_id, position, edge_port_no)) sts/traffic_generator.py: e.src = EthAddr(struct.pack("Q",self.random.randint(1,0xFF))[:6]) tests/unit/debugger/topology_generator_test.py: hw_addr=EthAddr("00:00:00:00:%02x:%02x" % (switch_id, port_no)) ) tests/unit/debugger/topology_generator_test.py: self.hosts = [Host([HostInterface(EthAddr("00:00:00:00:00:01"))], name="host1"), tests/unit/debugger/topology_generator_test.py: Host([HostInterface(EthAddr("00:00:00:00:00:02"))], name="host2")] tests/unit/debugger/topology_generator_test.py: hw_addr=EthAddr("00:00:00:00:%02x:%02x" % (switch_id, port_no)) ) tests/unit/headerspace/config_parser_test.py: print "int value is: ", EthAddr("00:00:11:22:33:00").toInt() tests/unit/headerspace/config_parser_test.py: print "string is:", str(EthAddr("00:00:11:22:33:00")) traces/ping_pong_fat_tree.trace:EthAddr traces/ping_pong_same_subnet.trace:EthAddr

[colin-scott]

Also, to be clear: that comment references you as having packed the struct in constructor in a certain way. That code was just mimicking what was in the constructor.

I agree that we could improve the code. Let's track the issue here, but this is not my highest priority right now.

[MurphyMc]

First, not that this is super important, but...

"Also, to be clear: that comment references you as having packed the struct in constructor in a certain way. That code was just mimicking what was in the constructor."

The comment you put says, "Murphy puts the least significant byte at [-1]". But I hadn't written the initial code, unless we're talking about totally separate things. Check the other referenced commit.

Secondly, I am unconvinced. Looking the relevant of those usages...

headerspace/config_parser/openflow_parser.py: #print "Eth String is:" + str(EthAddr(bytes_to_int(bytes))) Why convert bytes to an int? If the bytes are a bytes object or a string object or maybe a sequence, this should work anyway.

sts/traffic_generator.py: e.src = EthAddr(struct.pack("Q",self.random.randint(1,0xFF))[:6]) A generator expression could easily generate six bytes instead of a 64 bit int.

If you really do need to convert TO an integer, I think you should write an eth_to_int() function to do it in a way that makes sense for your application.

I'm planning to remove this functionality for the summer milestone.

[colin-scott]

OK. The comment wasn't meant to blame anyone. It was just pointing out that we needed to mimick the byte order of the ctor. Feel free to remove your name.

Those bytes are not the literal ethernet value. HSA has the notion of wildcard bits ("x"). So, each header bit can actually take on three values (0, 1, or x) in the internal representation. In order to represent three values, they need twice as many bits as the actual packet header. Feeding in the raw bytes to the EthAddr will yield incorrect results. Moreover, the only method HSA supports for converting the internal byte representation to a normal (no wildcard) value is by converting to an int.

[MurphyMc]

In that case, I suggest a bytes_to_bytes() function which takes HSA's representation and turns it into a 6-long bytes object instead of a bytes_to_int().

And it appears that where toInt() is used (openflow_parser.py), it's then immediately turned back into bytes. Skip the middleman.

Honestly, I am just trying to save everyone from the hassle: ethernet addresses in integers only leads to sadness!

[colin-scott]

Sounds like a good suggestion.

[andiwundsam]

+1 for Murphy's approach. Convert HSA's crazy representation into something that is useful, i.e., byte[6], then feed that into the POX API. Ints for storing byte-oriented data are horrible (small vs. big endian etc.).