fix packetOut validation and in_port enum reference#403
fix packetOut validation and in_port enum reference#403erickvermot wants to merge 2 commits intokytos:masterfrom erickvermot:packetout_fix
Conversation
| header = Header(message_type=Type.OFPT_PACKET_OUT) | ||
| buffer_id = UBInt32() | ||
| in_port = UBInt16() | ||
| in_port = UBInt16(enum_ref=Port) |
There was a problem hiding this comment.
in_port should not have an enum_ref, as you said in #364 (comment)
There was a problem hiding this comment.
true, I realized it after the PR, I'm fixing it, just a sec
diraol
left a comment
diraol
left a comment
There was a problem hiding this comment.
Another thing.
The _validate_in_port method seems to have a strange behavior to me.
Sometimes it just return, allowing in_port to have a integer value, sometimes it try to return an instance of Port (and even do another check of this value against _VIRT_IN_PORTS. Is this correct?
| if isinstance(port, Port): | ||
| if port not in _VIRT_IN_PORTS: | ||
| if (isinstance(self.in_port, int) and | ||
| (self.in_port > 0 and self.in_port < Port.OFPP_MAX.value)): |
There was a problem hiding this comment.
Port is a IntEnum, thus you do not need to use the .value here, it's elements can be directly compared to integers out of the box.
Also, why comparing if the in_port is less then Port.OFPP_MAX instead of less or equal to ?
In [4]: Port.OFPP_MAX
Out[4]: <Port.OFPP_MAX: 65280>
In [5]: 65280 == Port.OFPP_MAX
Out[5]: True
In [6]: 65280 >= Port.OFPP_MAX
Out[6]: True
In [7]: 65280 > Port.OFPP_MAX
Out[7]: False
In [8]: 65280 < Port.OFPP_MAX
Out[8]: False
In [9]: 65281 <= Port.OFPP_MAX
Out[9]: False
In [10]: 65279 <= Port.OFPP_MAX
Out[10]: TrueThere was a problem hiding this comment.
lines 116 to 118 treats valid ports that are not in Port class. the function returns so they don't get to be parsed to a instance of Port.
lines 119 to 125 treats valid ports that are in the Port class.
Since OFPP_MAX is in the Port class, we should let it be parsed.
I left the .value there because it was already there. (commit 7c7faf7)
cemsbr
left a comment
cemsbr
left a comment
There was a problem hiding this comment.
We need unit tests that fail in the current code and pass in this PR.
| if attribute_name == 'actions': | ||
| length = self.actions_len.value | ||
| attribute.unpack(buff[begin:begin+length]) | ||
| attribute.unpack(buff[begin:begin + length]) |
There was a problem hiding this comment.
This is was discussed in another PR and there should be no spaces around operator in list ranges.
There was a problem hiding this comment.
What pep8 relaxed rules say is that the spaces are optional. Not that "there should be no spaces".
I changed it anyway.
|
@macartur can you please try to understand this issue and make sure that all comments / changes requests are ok ? Thanks a lot. |
|
Packet_out validation is done with 6213565 - I'm closing this. |
6 participants
closes #402