MurphyMc
commented
8 years ago It sounds like the Brocade switches are not just "picky" but are in fact wrong. Or out-of-spec, anyway. I think what POX is doing is valid. Granted, this is an ugly corner of the spec which was underspecified in OpenFlow 1.0 and was addressed in OpenFlow 1.0.1, which says:
Protocol-specific fields within ofp_match must be ignored when the corresponding protocol is not specified in the match. The IP header and transport header fields must be ignored unless the Ethertype is specified as either IPv4 or ARP. The tp_src and tp_dst fields must be ignored unless the network protocol is set to TCP, UDP or ICMP. The dl_vlan_pcp field must be ignored when the OFPFW_DL_VLAN wildcard bit is set or when the dl_vlan value is set to OFP_VLAN_NONE. Fields that are ignored don’t need to be wildcarded and should be set to 0.
This language is still not as clear as it could be, but I think the only reasonable way to read it is that unless nw_proto is TCP, UDP, or ICMP, tcp_src and tp_dst must be ignored by the switch, and therefore should have values of zero and do not need to be wildcarded.
That said, I don't love this code myself, but it was originally added to support a usecase that I wasn't a part of and don't really know, so I've just left it alone for the most part. Additionally, I think how OVS may handle this same basic issue has also changed over time. Maybe it's time to revisit it and make a big change for eel.
As a sidenote, I think one piece of this code that I was involved in is that I believe it can be disabled globally by setting ofp_match.adjust_wildcards=False.
Getting down to it: you'd rather _unwire_wildcards() were called where _wire_wildcards() is called. I think equivalent to this is having fix() get called. I'd rather this was the default too. And I think current versions of OVS would be happier with this too (fewer log messages).
Given that the original use-case was a long time ago and I don't think it's still being worked on, my hasty reaction is:
- I think _unwire_wildcards() is just a less intuitive version of fix(), and should just be replaced with fix()
- fix() is a bad name and should be fix_wildcards()
- I think we can also replace _wire_wildcards() with fix() to get your desired behavior.
- To preserve the option of having the current behavior, fix() could be refactored slightly. Rather than always assigning None, it could assign some value. To get the old _wire_wildcards() behavior, we'd assign 0 instead of None, with a special case for nw_src and nw_dst. The ofp_match.adjust_wildcards attribute could be replaced with something that lets you select between three modes: leave wildcards alone, zero ignored fields (old default), wildcard ignored fields (new default).
Implications:
- Switching the default behavior may cause some regressions within POX (maybe in the POX datapath component in particular)
- Switching the default behavior may cause some regressions in other projects (STS is the most likely candidate, but they, unfortunately, stopped syncing to upstream POX a long time ago anyway)
- Three very similar functions which are supposed to stay in sync are replaced with a single function (seems valuable, since I actually broke the synchronization between them just a few days ago without thinking about it), and the two that are eliminated are, IMO, harder to read/less idiomatic.
Methods _wire_wildcards and _unwire_wildcards have the logic operation swapped. _wire_wildcards was supposed to clear fields whose type were not set. This should be done by clearing the fields value or setting the wildcard. Clearing the wildcard gives the opposite behavior. In the current implementation, an IPv4 match using the fields (in_port, dl_type=0x0800, nw_src, nw_dst) have the wildcards for TP_SRC and TP_DST forced to 0 while the fild vale is already 0 and thus breaking the compatibility with some picky devices llike Brocade switches.
I'm not sure how _unwire_wildcards should work, but if it is supposed to do have the reverse effect than _wire_wildcards, than the logic operations from each method should be swapped.