Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes under src/ptf for python3 compatible changes #106

Merged
merged 14 commits into from
Jan 8, 2020
Merged

Conversation

saynb
Copy link
Contributor

@saynb saynb commented Jan 1, 2020

Major changes include

  1. Running lib2to3 on .py files under src/ptf
  2. Changing manually str() to bytes() internally on pkt objects. In python2, bytes() and str() is the same thing but in python3 str() will convert it into a unicode object.

Copy link

@cc10512 cc10512 left a comment

Choose a reason for hiding this comment

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

I'm afraid this is not fully tested, as there are still typos. How are you validating that your changes?
You should at least add unit tests that exercise simple_ipv4ip_packet and simple_ipv6ip_packet. The more unit tests that cover your changes, the better.

import mask
from . import ptfutils
from . import netutils
from . import mask
Copy link

Choose a reason for hiding this comment

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

import .ptfutils works as well and it is more readable in my view.

Copy link

Choose a reason for hiding this comment

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

If you want to keep compatibility with Python2 (as below), you need the following:

if sys.version_info.major == 2:
    import ptfutils
else:
    import .ptfutils```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicit relative import is not supported in python 3. However, explicit relative import is supported in both python 2 and 3 so the import style from . import ptfutils still works. The import style import .ptfutils is not supported in python2. So rather than littering the entire codebase with p2 vs p3 imports we can just choose the common supported one.
One thing that I missed earlier is that in python 3, imports inside function level has been discouraged now with deprecation warnings. In order to get use of lazy importing, we need to use importlib which is unnecessary for the usecases here

src/ptf/dataplane.py Outdated Show resolved Hide resolved
src/ptf/dataplane.py Show resolved Hide resolved
src/ptf/testutils.py Outdated Show resolved Hide resolved
@@ -859,7 +859,8 @@ def simple_gre_packet(pktlen=300,

if inner_frame:
pkt = pkt / inner_frame
if ((ord(str(inner_frame)[0]) & 0xF0) == 0x60):
inner_frame_bytes = bytearray(bytes(inner_fram))
Copy link

Choose a reason for hiding this comment

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

typo: inner_frame.
Also, AFAICT, you can directly construct a bytearray, without first converting to bytes: https://docs.python.org/3.7/library/stdtypes.html?highlight=bytearray#bytearray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it with just bytearray() directly but if inner_frame is a scapy pkt object, then it won't work. So converting it to bytes first is a safe way of converting to a common representation.

* Correcting utests and nn-tests to run on python3
Copy link
Contributor Author

@saynb saynb left a comment

Choose a reason for hiding this comment

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

Still testing out these changes on both python2 and python3 while adding more to unit tests. I raised the PR early to get some greatly appreciated feedback. Thanks!

import mask
from . import ptfutils
from . import netutils
from . import mask
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicit relative import is not supported in python 3. However, explicit relative import is supported in both python 2 and 3 so the import style from . import ptfutils still works. The import style import .ptfutils is not supported in python2. So rather than littering the entire codebase with p2 vs p3 imports we can just choose the common supported one.
One thing that I missed earlier is that in python 3, imports inside function level has been discouraged now with deprecation warnings. In order to get use of lazy importing, we need to use importlib which is unnecessary for the usecases here

src/ptf/dataplane.py Outdated Show resolved Hide resolved
src/ptf/dataplane.py Show resolved Hide resolved
@@ -2480,7 +2484,7 @@ def ptf_ports(num=None):
return ports[:num]

def port_to_tuple(port):
if type(port) is int or type(port) is long:
if type(port) is int or type(port) is int:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change this to check for long when python 2

* Correcting pktlen issue where a byte with value > 127 doubled the bytes
due to default encoding being unicode on python3
* Correcting hexdump()
Copy link

@cc10512 cc10512 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests.

@saynb
Copy link
Contributor Author

saynb commented Jan 7, 2020

I have added a few tests but the ones with SimpleIpv4 are failing on python2 with a pktlength of >74. For now, I have made the pktlen to be 70 for now so that the test passes. I have checked in another branch https://github.com/p4lang/ptf/tree/ipv
which only adds a simple IPv4 test with a pktlen of 75 on top of master and the travis job fails. It mostly looks like some kind of nanomsg/nnpy issue where some extra bytes are being added. @antoninbas Do you know of anything about this issue?

@saynb saynb merged commit 361e659 into master Jan 8, 2020
@saynb saynb deleted the python2to3 branch January 8, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants