gh-124944: Add socket.SO_ORIGINAL_DST#124945
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
10b2c9a to
32f9269
Compare
Misc/NEWS.d/next/Library/2024-10-03-17-13-22.gh-issue-124944.YyLAzf.rst
Outdated
Show resolved
Hide resolved
|
Also, you need to test this attribute in the corresponding test. |
|
@rruuaanng do you have any ideas on how to write a meaningful test? The test should only run if the system has netfilter support. def _have_socket_bluetooth():
"""Check whether AF_BLUETOOTH sockets are supported on this host."""
try:
s = socket.socket(socket.AF_BLUETOOTH, socket.SOCK_STREAM, socket.BTPROTO_RFCOMM)
except (AttributeError, OSError):
return False
else:
s.close()
return True
# ...
HAVE_SOCKET_BLUETOOTH = _have_socket_bluetooth()And then we run tests only if there is bluetooth support @unittest.skipUnless(HAVE_SOCKET_BLUETOOTH,
'Bluetooth sockets required for this test.')
class BasicBluetoothTest(unittest.TestCase):
def testBluetoothConstants(self):
socket.BDADDR_ANY
socket.BDADDR_LOCAL
socket.AF_BLUETOOTH
socket.BTPROTO_RFCOMM
# ...So the bluetooth tests are a bit circular because we test for However, in the case of if hasattr(socket, "SO_ORIGINAL_DST"):
assert hasattr(socket, "SO_ORIGINAL_DST")which is dead code. |
32f9269 to
73b2630
Compare
Actually, you can integrate this code into the existing tests for the sockets attribute, rather than writing a new test. It's meaning(Maybe). Edit: And,It seems that Edit: Maybe you should to see |
|
@rruuaanng My understanding is that
And although most Linux distributions have netfilter and related headers (I couldn't find one without), Linux does not imply netfilter/ if sys.platform.startswith('linux'):
socket.SO_ORIGINAL_DSTas tests in After writing all that, I do think that we can check whether if hasattr(socket, "SO_ORIGINAL_DST"):
s = # set up socket
opt = getsockopt(s, socket.SOL_IP, socket.SO_ORIGINAL_DST)
# check that `opt` makes senseThis way we don't assert that linux systems have netfilter, but we do say that if Let me know what you think |
|
You can actually add a description of this attribute in the documentation. |
Misc/NEWS.d/next/Library/2024-10-03-17-13-22.gh-issue-124944.YyLAzf.rst
Outdated
Show resolved
Hide resolved
|
I don't think there's need to test this since we don't test other non-crucial constants, nor is there a need to add a dedicated documented entry (the docs just gather most constants under If you want a test, you can just do something like this: @unittest.skipUnless(sysconfig.get_config_var('HAVE_LINUX_NETFILTER_IPV4_H'),
"requires <linux/netfilter_ipv4.h>")
def test_socket_linux_netfilter_ipv4(self):
socket.SO_ORIGINAL_DST |
cb6f2e3 to
d417182
Compare
d417182 to
ed669e0
Compare
|
going to ignore the docs error because it also happens for |
Misc/NEWS.d/next/Library/2024-10-03-17-13-22.gh-issue-124944.YyLAzf.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Bénédict, are you happy with the changes? @picnixz |
|
Let's go! Thanks, @Stevenjin8. |
#124944