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

Pluggable packet module proposal #147

Merged
merged 9 commits into from
Nov 30, 2021
Merged

Pluggable packet module proposal #147

merged 9 commits into from
Nov 30, 2021

Conversation

sborkows
Copy link
Contributor

Now, packet module will be pluggable, as it will try do dynamically
import an implementation specified in config or provided as an argument.
In that way PTF will become independent regarding Scapy, since one can
develop his/her own packet manipulation framework and then create an
implementation of packet module for it. By default it will be Scapy.

Now, packet module will be pluggable, as it will try do dynamically
import an implementation specified in config or provided as an argument.
In that way PTF will become independent regarding Scapy, since one can
develop his/her own packet manipulation framework and then create an
implementation of packet module for it. By default it will be Scapy.
@NexSabre
Copy link
Contributor

Can you provide some examples, how we should use it with dpkt? Also, instruction and examples should be added to README.md.

ptf Outdated Show resolved Hide resolved
src/ptf/packet.py Outdated Show resolved Hide resolved
Addressed community review

- Added short (-pmm) arg option
- Provided some description in README.md
- Added default value for packet manipulation module in generic packet
  module
- Removed scapy from requirements.txt as it is now optional, if custom
  packet manipulation module is provided
- Removed alias 'scapy' from testutils
Copy link
Contributor

@NexSabre NexSabre left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Yi-Tseng Yi-Tseng left a comment

Choose a reason for hiding this comment

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

LGTM overall, would be nice to have a unit test that uses a different packet framework

@sborkows
Copy link
Contributor Author

ATM I'm testing this solution internally, as 'test' pipeline cannot be run yet.

@antoninbas would you mind looking at these proposal?

* Add hexdump symbol to packet module

Also, modified mask module to not call Scapy directly.

* Increase verbosity when choosing PMM

* Deprecate set_do_not_care_scapy

Now please use set_do_not_care_packet instead.
src/ptf/mask.py Show resolved Hide resolved
@@ -84,14 +90,14 @@ def __str__(self):


def utest():
Copy link
Contributor

Choose a reason for hiding this comment

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

This test can be transferred to a separate module (test), it is not needed here. Instead, you can add a short comment to the class

* Changes in dataplane to become scapy independent

* Fix name for ERSPAN platform specific in testutils
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ptf Outdated Show resolved Hide resolved
src/ptf/packet.py Show resolved Hide resolved
Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas antoninbas merged commit de2b4b3 into p4lang:master Nov 30, 2021
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.

None yet

4 participants