Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

implement general unpack method (moved from of_core.utils) #397

Closed
wants to merge 2 commits into from
Closed

implement general unpack method (moved from of_core.utils) #397

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9842dc
introduce general unpack method (comes from of_core.utils)
erickvermot Jul 7, 2017
a2d4480
separate packet validation from unpack
erickvermot Jul 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions pyof/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
"""General Unpack utils for python-openflow."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add a comment stating that you are copying the code from of_core, specifying the filename from its repo root. Also write that this is because you want to have the exact same behavior for testing purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is already clear in the corresponding issues mentioned here:
#396 and #381

import pyof.v0x01.common.header
import pyof.v0x01.common.utils
import pyof.v0x04.common.header
import pyof.v0x04.common.utils
Copy link
Contributor

Choose a reason for hiding this comment

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

@erickvermot you are not using this imported modules anywhere, so you should not import them here.
Considering your code, I would do:

from pyof import v0x01
from pyof import v0x04

pyof_version_libs = {0x01: v0x01,
                     0x04: v0x04}

Note that I'm using the from statement, instead of the import statement.
This is because on some tests I've performed related to the python importing system, this import statement appeared to be less error prone than using the from ... import statement, so I would give preference to use the latter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even better:
from pyof import v0x01, v0x04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anywhere = line 25 for example. Those imports are needed since importing pyof does not import the child modules by default.

The from statement is better suited when one tries to import a Class, function or variable from a given module, or when one wants to place the imported module in the current namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked this out and, unfortunately, Diraol suggestion doesn't work because this is a special case like Erick said. Linters can't detect unused import statements like these, but I can't think of a better way to do it right now.

Copy link
Contributor Author

@erickvermot erickvermot Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

  • "unused import statements like these", That's because they are not unused, as I said before.

  • This is not a special case. Unless we make the imports inside the init s, those imports are needed.

from pyof.foundation.exceptions import UnpackException

pyof_version_libs = {0x01: pyof.v0x01,
0x04: pyof.v0x04}


def unpack(packet):
"""Unpack the OpenFlow Packet and returns a message."""
try:
version = packet[0]
except IndexError:
raise UnpackException('null packet')

try:
pyof_lib = pyof_version_libs[version]
except KeyError:
raise UnpackException('Version not supported')

try:
header = pyof_lib.common.header.Header()
header.unpack(packet[:8])
Copy link
Contributor

Choose a reason for hiding this comment

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

@erickvermot the header will always have length 8 [just to make sure that on OF 1.3.5 and newer they still keep this behavior]?
Or should we use pyof_lib.common.header.Header.get_size() instead of 8?

If my comment make any sense, then we should "apply" it below also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

headers always have length 8

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok then!

message = pyof_lib.common.utils.new_message_from_header(header)
binary_data = packet[8:]
if binary_data:
if len(binary_data) == header.length - 8:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for adding this check to the of_core code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

packet (not message) validation.
In of_core this is also done, just not inside the unpack function.

message.unpack(binary_data)
else:
raise UnpackException(
'packet size does not match packet length field')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we always call unpack, Even if we have no binary_data?

If the specific Message does have a "body" it may always wait for its body. If no body was passed, then the unpack will fail - and it should! Eventually, if the "body" is optional (or some attributes are), then the unpack should handle it naturally without raising an exception.

If the specific message does not have a body, then the binary-data is expected to be "empty", therefore the unpack should not have any problem being called.

On the current code we are "ignoring" when the Message (class) does have a mandatory body and the message was sent through the network just with the header and without the body, which shouldn't be accepted while doing the unpack, right? Does it make sense?

Considering my comment, it seems that this block can be something like

message.unpack(binary_data)

And I would also do the check len(binary_data) == header.length - 8 within the "unpack" method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This unpack function unpacks all the data there is.
It does not perform message validation. Which is currently not implemented in pyof (there are many issues opened about it).

About what the block does:

  • If there is no data to unpack, then there is no reason to make the call.
  • If the length of the data is different of whats in the header.length field, then there is no reason to make the call either.

+ this PR is about bringing the method from of_core to pyof. We need this to have more reliable tests. There are still many improvements to this function, but I plan to make them in a different PR (regarding version agnostic messages for ex like the hello msg, as we have discussed before).

Copy link
Contributor

Choose a reason for hiding this comment

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

@diraol, @erickvermot told me something before that you might not be aware (see my review comment to write it in the module docstring): the purpose of this PR is to provide the same unpack of of_core to python-openflow tests. Thus, my review is based on copying the unpack method. Maybe we should move discussions about changing the current of_code code to another issue, wherever it will be (we must remove this duplication).

Regarding Erick's 2 bullets, I see those issues in a different way, respectively:

  • If there's no data to unpack, it's dangerous to assume that it will always mean nothing. It should be up to each message to decide it. Thus, I believe we'd better always call unpack.
  • This should be done in the message validation. If it is not done yet, I see no reason on implementing it quick and dirty in the wrong place.

Copy link
Contributor Author

@erickvermot erickvermot Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

About the first parag:
This is stated in the title and in the mentioned issues

About the bullets, you are mixing message validation with packet validation.
Unpack method deals with the packet, so packet validation is important so it doesn't mess thing around. Message validation should not be done here.
So:

  • if the packet is valid and there is nothing more to unpack, there is no reason to make the call.
  • If the packet is invalid, it should stop unpacking. This is not 'quick and dirty', you just need to understand it better

return message
except (UnpackException, ValueError) as e:
raise UnpackException(e)