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

erickvermot · 2017-07-10T14:47:31Z

relates to #381

also provides way to #396

diraol · 2017-07-10T17:23:59Z

@beraldoleal should we create a utils module under pyof and place this unpack utility over there?

from pyof.utils import unpack for example.

erickvermot · 2017-07-10T17:32:13Z

@diraol @beraldoleal, I believe it could be in the main module:

import pyof
pyof.unpack()

beraldoleal · 2017-07-11T12:57:52Z

@diraol @erickvermot for now is better on utils. like @diraol said.

erickvermot · 2017-07-11T20:01:54Z

@diraol, @beraldoleal, done.

diraol · 2017-07-12T12:01:41Z

pyof/utils.py

+import pyof.v0x01.common.header
+import pyof.v0x01.common.utils
+import pyof.v0x04.common.header
+import pyof.v0x04.common.utils


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

Even better:
from pyof import v0x01, v0x04

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.

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.

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

diraol · 2017-07-12T12:06:47Z

pyof/utils.py

+
+    try:
+        header = pyof_lib.common.header.Header()
+        header.unpack(packet[:8])


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

headers always have length 8

diraol · 2017-07-12T12:21:46Z

pyof/utils.py

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


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.

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

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

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

diraol

I don't know why GH didn't marked the 'resquest changes' status..

cemsbr · 2017-07-13T19:48:21Z

pyof/utils.py

+import pyof.v0x01.common.header
+import pyof.v0x01.common.utils
+import pyof.v0x04.common.header
+import pyof.v0x04.common.utils


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.

cemsbr · 2017-07-13T20:38:04Z

pyof/utils.py

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


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

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

cemsbr · 2017-07-13T20:43:38Z

pyof/utils.py

@@ -0,0 +1,37 @@
+"""General Unpack utils for python-openflow."""


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.

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

cemsbr · 2017-07-13T21:27:12Z

pyof/utils.py

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


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

erickvermot · 2017-07-13T22:10:20Z

@cemsbr, you duplicated a comment, but comments about the bullets are in the review answer: #397 (comment)

erickvermot · 2017-07-18T02:16:55Z

should I include the following mod in this PR?
master...erickvermot:general_unpack_new

beraldoleal · 2017-09-11T14:36:07Z

@macartur can you please try to understand this issue and make sure that all comments / changes requests are ok ? Thanks a lot.

cemsbr · 2017-09-20T13:47:54Z

Implemented in #444.

diraol added enhancement level:easy pri:important labels Jul 11, 2017

diraol assigned erickvermot Jul 11, 2017

diraol requested review from cemsbr and diraol July 11, 2017 14:34

diraol added this to the 2017.2 milestone Jul 11, 2017

introduce general unpack method (comes from of_core.utils)

e9842dc

erickvermot mentioned this pull request Jul 11, 2017

Teststruct improvement application #404

Closed

diraol reviewed Jul 12, 2017

View reviewed changes

diraol removed this from the 2017.2 milestone Jul 12, 2017

diraol suggested changes Jul 12, 2017

View reviewed changes

erickvermot requested a review from renanrodrigo July 13, 2017 18:12

erickvermot mentioned this pull request Jul 13, 2017

Teststruct improvement #406

Closed

cemsbr suggested changes Jul 13, 2017

View reviewed changes

separate packet validation from unpack

a2d4480

macartur self-assigned this Sep 12, 2017

cemsbr closed this Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erickvermot commented Jul 10, 2017

diraol commented Jul 10, 2017

erickvermot commented Jul 10, 2017 •

edited

Loading

beraldoleal commented Jul 11, 2017

erickvermot commented Jul 11, 2017

diraol Jul 12, 2017

diraol Jul 12, 2017

erickvermot Jul 12, 2017

cemsbr Jul 13, 2017

erickvermot Jul 13, 2017 •

edited

Loading

diraol Jul 12, 2017

erickvermot Jul 12, 2017

diraol Jul 13, 2017

diraol Jul 12, 2017

erickvermot Jul 12, 2017

cemsbr Jul 13, 2017

erickvermot Jul 13, 2017 •

edited

Loading

diraol left a comment

cemsbr Jul 13, 2017

cemsbr Jul 13, 2017

erickvermot Jul 13, 2017

cemsbr Jul 13, 2017

erickvermot Jul 13, 2017

cemsbr Jul 13, 2017

erickvermot commented Jul 13, 2017

erickvermot commented Jul 18, 2017

beraldoleal commented Sep 11, 2017

cemsbr commented Sep 20, 2017

		@@ -0,0 +1,37 @@
		"""General Unpack utils for python-openflow."""

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

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

Conversation

erickvermot commented Jul 10, 2017

diraol commented Jul 10, 2017

erickvermot commented Jul 10, 2017 • edited Loading

beraldoleal commented Jul 11, 2017

erickvermot commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickvermot Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickvermot Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

diraol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erickvermot commented Jul 13, 2017

erickvermot commented Jul 18, 2017

beraldoleal commented Sep 11, 2017

cemsbr commented Sep 20, 2017

erickvermot commented Jul 10, 2017 •

edited

Loading

erickvermot Jul 13, 2017 •

edited

Loading

erickvermot Jul 13, 2017 •

edited

Loading