Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-96534: socketmodule: support FreeBSD divert(4) socket#96536
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedSep 3, 2022
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
ghost commentedSep 3, 2022 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
bedevere-bot commentedSep 3, 2022
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
I see no problem with it, as it's just constants. But should we have a test? |
Do you have FreeBSD in your CI? Is it worth to have one just for this small case? I can ensure you that this code will be exercised in our FreeBSD CI, as we use python for regression test of the divert module :) |
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 don't get the point of this PR.AF_DIVERT andPF_DIVERT don't exist my FreeBSD 13 VM (grep doesn't match any line):
vstinner@freebsd$ grep AF_DIVERT -R /usr/include/ vstinner@freebsd$ grep PF_DIVERT -R /usr/include/But IPPROTO_DIVERT is defined:
vstinner@freebsd$ grep IPPROTO_DIVERT -R /usr/include/ /usr/include/netinet/in.h:#define IPPROTO_DIVERT 258 /* divert pseudo-protocol */And the manual page mentioned in the Python issue contains:
int socket(PF_INET, SOCK_RAW, IPPROTO_DIVERT);For me, IPPROTO_DIVERT constant should be added, no?
bedevere-bot commentedNov 2, 2022
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
FreeBSD divert manual page:https://www.freebsd.org/cgi/man.cgi?divert |
There are FreeBSD buildbot workers, but I don't think that it's worth it to add an unit test (I'm fine with merging the PR without a test). |
The new protocol family exists only in FreeBSD 14 and the old PF_INET/IPPROTO_DIVERT will go away in FreeBSD 14: All known open source software is already patched wrt this change. The way I patched the Python's socketmodule allows it to be compiled both on FreeBSD 14 and old versions correctly, so that new builds will support PF_DIVERT and old builds will allow to use PF_INET with protocol integer equal to IPPROTO_DIVERT, just as they did before. |
|
Ah, I tested your PR on a FreeBSD 13 VM. When is the FreeBSD 14 release planned? I understand that his PR is not usable on FreeBSD 13, right? Would it be useful to expose IPPROTO_DIVERT for FreeBSD 13? |
The currenthttps://cgit.freebsd.org/src/plain/tests/sys/common/divert.py script mentioned in the issue uses IPPROTO_DIVERT (by defining it manually to 258). |
| PyModule_AddIntMacro(m, PF_DIVERT); | ||
| #endif | ||
| #ifdef AF_DIVERT | ||
| PyModule_AddIntMacro(m, AF_DIVERT); |
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.
It would be nice to add a comment why we need to check for both PF_DIVERT and
AF_DIVERT and mention the related freebsd versions.
You explained this in the discussion but having this in the code will make it much
easier to maintain 5 years from 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 distinction betweenprotocol families (PF_* constants) andaddress families (AF_* constants) is what we inherited from the original BSD sockets API. The PF_FOO is supposed to be an argument for socket(2), while AF_FOO for any socket address manipulation syscalls or functions, e.g. bind(), getsockname(), etc. This distinction makes some sense, since existence of a protocol doesn't dictate existence of address type and vice versa. The divert(4) is actually nice example, as it uses AF_INET addresses. Or look at AF_RDS a couple lines above in the socketmodule.c - the same.
However, all modern implementations of sockets always declare both AF_FOO and PF_FOO. As far as I know, they always are equal. POSIXdoesn't mention PF_* families. Linuxnotes their existence in the NOTES of the manual page. Many software developers would use AF_FOO when calling socket(). This is why AF_DIVERT is also provided by FreeBSD and this is why I provided PyModule_AddIntMacro for it, too.
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.
Right, looking at the rest of the code it is expected to have both constants.
| #ifdef AF_DIVERT | ||
| case AF_DIVERT: | ||
| /* FreeBSD divert(4) sockets use sockaddr_in: fall-through */ | ||
| #endif /* AF_DIVERT */ |
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 wonder why we don't need to check for PF_DIVERT here? Do we need a comment for this?
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.
Neither PF_DIVERT of AF_DIVERT are provided on old versions of FreeBSD. Both are provided on the new versions. I think this is stylistically more correct to put ifdefs as I did. For longer explanation on the difference between them, see my other comment.
| @@ -0,0 +1 @@ | |||
| Support divert(4) on FreeBSD 14. | |||
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.
Only on 14?
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.
Well, 14 and beyond to be precise.
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.
Maybe "Support divert(4) added in FreeBSD 14."?
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.
Pushed that!
cc@koobs |
koobs commentedNov 2, 2022
koobs commentedNov 2, 2022
The 12.x buildbot worker will verify/confirm that |
It is planned for summer 2023.
Right, on FreeBSD 13 the new code won't be compiled in. I don't think there is any reason to add support for obsolete constant. To my knowledge our regression suite CI is the only Python code that uses it. |
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.
Looks good to me.
It would be nice to have tests using the new constants when they are available.
| PyModule_AddIntMacro(m, PF_DIVERT); | ||
| #endif | ||
| #ifdef AF_DIVERT | ||
| PyModule_AddIntMacro(m, AF_DIVERT); |
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.
Right, looking at the rest of the code it is expected to have both constants.
| @@ -0,0 +1 @@ | |||
| Support divert(4) on FreeBSD 14. | |||
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.
Maybe "Support divert(4) added in FreeBSD 14."?
koobs commentedNov 3, 2022
Addressed in#96536 (comment), no longer awaiting changes |
They are users running FreeBSD older than FreeBSD 14. Without IPPROTO_DIVERT exposed in Python, this kind of socket cannot be used in Python. Well, if your plan is to only add support for divert sockets on FreeBSD 14, I will wait until FreeBSD 14 is released to manually test this PR. |
Nothing changes to users of older FreeBSD with my change. The constant wasn't defined before and it stays undefined. The old style socket could actually be used, just using 258 literal. However, the new can not be used with this hack, due to bind() failing due to socketmodule not recognizing the address family. Seethis change.
You can install this VM image from herehttps://download.freebsd.org/snapshots/VM-IMAGES/14.0-CURRENT/amd64/ Either date shall work, they all are new enough. I'd really appreciate if this happens soon rather than wait for 14.0-RELEASE. This will make our testing easier and will allow to faster obsolete the old socket. |
Can't you have a downstream patch in FreeBSD to test your change before FreeBSD 14 is released? That's what we do in Fedora to unblock issues before upstream accepts our patches (or when the change is merged but not released yet). I would prefer to wait until FreeBSD 14 is released, before making changes to support it. By the way, Python 3.12 is not going to be released before October 2023 (in one year). |
Can you please propose a PR just to add IPPROTO_DIVERT? I could test it immediately. |
Now all Python ports has been patched to support PF_DIVERT, andPython kinda promises to add support in 3.12 [1].This reverts commit322b5b7.[1]python/cpython#96536 (comment)
glebius commentedFeb 9, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Status update: mmkay, I have added this patch to all python ports on FreeBSD. Victor, I hope you will merge it once FreeBSD 14.0-RELEASE is officially out. |
Now all Python ports has been patched to support PF_DIVERT, andPython kinda promises to add support in 3.12 [1].This reverts commit322b5b7.[1]python/cpython#96536 (comment)
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.
Would it be possible to somehow document these constants? See for example:
https://docs.python.org/dev/library/socket.html#socket.AF_RDS
Document that they are new in Python 3.12.
I understand that you have this downstream patch in FreeBSD CURRENT for a few weeks/months. I'm now fine with adding these FreeBSD 14 constants (once the review will be done). |
@vstinner updated socket module documentation as you suggested. Let me know if I need anything else to be done to get this in. |
@vstinner We are entering FreeBSD 14 release cycle. I'd like to get this in python before FreeBSD 14.0 is released. Let me know how I can help to achieve 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.
LGTM. Thanks for adding the documentation.
Better late than never, I merged your PR. Thanks for your contribution. |
koobs commentedMay 5, 2023
Thanks Victor! ❤️ |
* main: (61 commits)pythongh-64595: Argument Clinic: Touch source file if any output file changed (python#104152)pythongh-64631: Test exception messages in cloned Argument Clinic funcs (python#104167)pythongh-68395: Avoid naming conflicts by mangling variable names in Argument Clinic (python#104065)pythongh-64658: Expand Argument Clinic return converter docs (python#104175)pythonGH-103092: port `_asyncio` freelist to module state (python#104196)pythongh-104051: fix crash in test_xxtestfuzz with -We (python#104052)pythongh-104190: fix ubsan crash (python#104191)pythongh-104106: Add gcc fallback of mkfifoat/mknodat for macOS (pythongh-104129)pythonGH-104142: Fix _Py_RefcntAdd to respect immortality (pythonGH-104143)pythongh-104112: link from cached_property docs to method-caching FAQ (python#104113)pythongh-68968: Correcting message display issue with assertEqual (python#103937)pythonGH-103899: Provide a hint when accidentally calling a module (pythonGH-103900)pythongh-103963: fix 'make regen-opcode' in out-of-tree builds (python#104177)pythongh-102500: Add PEP 688 and 698 to the 3.12 release highlights (python#104174)pythonGH-81079: Add case_sensitive argument to `pathlib.Path.glob()` (pythonGH-102710)pythongh-91896: Deprecate collections.abc.ByteString (python#102096)pythongh-99593: Add tests for Unicode C API (part 2) (python#99868)pythongh-102500: Document PEP 688 (python#102571)pythongh-102500: Implement PEP 688 (python#102521)pythongh-96534: socketmodule: support FreeBSD divert(4) socket (python#96536) ...
Uh oh!
There was an error while loading.Please reload this page.
All rationale in#96534