-
Notifications
You must be signed in to change notification settings - Fork 100
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
Replace netutils.py with from-scratch implementation under Apache 2.0 license #211
Replace netutils.py with from-scratch implementation under Apache 2.0 license #211
Conversation
… license Signed-off-by: Andy Fingerhut <[email protected]>
Intended to address issue #210 I will update with another comment after I have used this version to run ptf tests in all of these places, with my test results:
I plan to test on at least Ubuntu 20.04 and Ubuntu 24.04, combined with {x86_64, aarch64} processor architectures. |
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Updates: PTF tests are passing with these changes on these combos:
It does require doing Except for that, I think this is ready to review, and merge if approved. |
src/ptf/netutils.py
Outdated
PACKET_ADD_MEMBERSHIP = 1 | ||
PACKET_DROP_MEMBERSHIP = 2 | ||
PACKET_MR_PROMISC = 1 | ||
import netifaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we should need an unmaintained dependency for a simple task like this. I rather we write these functions from scratch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, no p4lang repository code is actually calling the get_mac
function anywhere. If you prefer, I can just delete it, and then there is no reason to import netifaces
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I could copy this Apache 2.0 implementation in this repo for get_mac into the file netutils.py
, just in case someone does call get_mac
in other repos that are not in the p4lang Github org: https://github.com/p4lang/ptf/blob/main/ptf_nn/ptf_nn_agent.py#L51-L65
Let me try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks good. For testing, ideally you want to run this on the P4C tests.
This is what Gemini gives me. I think LLMs are a good use case for straightforward cases like this one.
import socket
import struct
import fcntl
# Given 'iff', the name of a network interface (e.g. 'veth0') as a
# string, return a string of the form '00:00:00:00:00:00' containing
# the MAC address of that interface, where all digits are hexadecimal.
def get_mac(iff: str) -> str:
try:
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
info = fcntl.ioctl(s.fileno(), 0x8927, struct.pack('256s', iff[:15].encode()))
mac = ':'.join(['{:02x}'.format(x) for x in info[18:24]])
return mac
except Exception as e:
print(f"Error getting MAC address: {e}")
return "00:00:00:00:00:00"
# Given 'iff', the name of a network interface (e.g. 'veth0') as a
# string, and 's', a SOCK_RAW socket bound to that interface, set the
# interface in promiscuous mode if parameter 'val' != 0, or into
# non-promiscuous mode if 'val' == 0.
def set_promisc(s, iff: str, val=1):
try:
if val:
# Set promiscuous mode
ifr = struct.pack('16sH', iff.encode(), socket.htons(socket.IF_PROMISC))
fcntl.ioctl(s.fileno(), socket.SIOCSIFFLAGS, ifr)
else:
# Set non-promiscuous mode
ifr = struct.pack('16sH', iff.encode(), socket.htons(0))
fcntl.ioctl(s.fileno(), socket.SIOCSIFFLAGS, ifr)
except Exception as e:
print(f"Error setting promiscuous mode: {e}")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your first try with an LLM gave me failures, so I looked elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this implementation of set_promisc with the full p4c test suite, on my local system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this implementation of set_promisc with the full p4c test suite, on my local system.
by "this implementation" I mean not the one produced by the LLM, but the one that is in this PR now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way we ideally should test this is to set the PTF dependency to a Github URL here:
https://github.com/p4lang/p4c/blob/main/requirements.txt#L5
Using this approach: https://stackoverflow.com/a/35998253
Then we can update and test PTF directly on the P4C repository. Let me look into how to set this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly have no objections if you want to test this in a different way than I already have. There is no rush on merging this. Just trying to keep the ball rolling on letting us license ptf as Apache-2.0 (or at least not copyleft) with a straight face.
Signed-off-by: Andy Fingerhut <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving considering a lot of tests pass. I am not sure how these files are exactly exercised in the PTF framework.
No description provided.