gh-96534: socketmodule: support FreeBSD divert(4) socket #96536

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 :)
This change just waits to be enabled back in our CI as soon as python accepts this pull request and new python version hits FreeBSD ports.

vstinner

I don't get the point of this PR. AF_DIVERT and PF_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 · 2022-11-02T21:12:46Z

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2022-11-02T21:13:15Z

FreeBSD divert manual page: https://www.freebsd.org/cgi/man.cgi?divert

vstinner · 2022-11-02T21:16:32Z

Do you have FreeBSD in your CI?

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).

glebius · 2022-11-02T21:28:30Z

I don't get the point of this PR. AF_DIVERT and PF_DIVERT don't exist my FreeBSD 13 VM (grep doesn't match any line):
..
For me, IPPROTO_DIVERT constant should be added, no?

The new protocol family exists only in FreeBSD 14 and the old PF_INET/IPPROTO_DIVERT will go away in FreeBSD 14:

https://cgit.freebsd.org/src/commit/sys/netinet/ip_divert.c?id=8624f4347e8133911b0554e816f6bedb56dc5fb3

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.

glebius · 2022-11-02T21:43:48Z

FreeBSD divert manual page: https://www.freebsd.org/cgi/man.cgi?divert

FreeBSD 14 divert manual page

vstinner · 2022-11-02T21:52:57Z

FreeBSD 14 divert manual page

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?

vstinner · 2022-11-02T21:54:19Z

The current https://cgit.freebsd.org/src/plain/tests/sys/common/divert.py script mentioned in the issue uses IPPROTO_DIVERT (by defining it manually to 258).

nirs · 2022-11-02T21:51:47Z

Modules/socketmodule.c

+    PyModule_AddIntMacro(m, PF_DIVERT);
+#endif
+#ifdef AF_DIVERT
+    PyModule_AddIntMacro(m, AF_DIVERT);


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.

The distinction between protocol families (PF_* constants) and address 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. POSIX doesn't mention PF_* families. Linux notes 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.

Right, looking at the rest of the code it is expected to have both constants.

nirs · 2022-11-02T21:52:45Z

Modules/socketmodule.c

+#ifdef AF_DIVERT
+    case AF_DIVERT:
+        /* FreeBSD divert(4) sockets use sockaddr_in: fall-through */
+#endif /* AF_DIVERT */


I wonder why we don't need to check for PF_DIVERT here? Do we need a comment for this?

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.

nirs · 2022-11-02T21:54:10Z

Misc/NEWS.d/next/Library/2022-09-03-09-24-02.gh-issue-96534.EU4Oxv.rst

@@ -0,0 +1 @@
+Support divert(4) on FreeBSD 14.


Only on 14?

Well, 14 and beyond to be precise.

Maybe "Support divert(4) added in FreeBSD 14."?

Pushed that!

vstinner · 2022-11-02T21:57:08Z

cc @koobs

koobs · 2022-11-02T22:07:36Z

cc @koobs

We have my buildbot workers for 12.x and CURRENT, so QA/CI isn't an issue.

@glebius if this landed only recently (If its a more recent CURRENT than a few months), I'll need to update (there's been a zfs arc regression preventing upgrade) the worker first.

koobs · 2022-11-02T22:08:46Z

The way I patched the Python's socketmodule allows it to be compiled both on FreeBSD 14 and old versions correctly,

The 12.x buildbot worker will verify/confirm that

vstinner · 2022-11-02T22:20:27Z

cc @Jehops @emaste

glebius · 2022-11-02T22:21:42Z

Ah, I tested your PR on a FreeBSD 13 VM. When is the FreeBSD 14 release planned?

It is planned for summer 2023.

I understand that his PR is not usable on FreeBSD 13, right? Would it be useful to expose IPPROTO_DIVERT for FreeBSD 13?

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.

nirs

Looks good to me.

It would be nice to have tests using the new constants when they are available.

nirs · 2022-11-02T22:32:22Z

Modules/socketmodule.c

+    PyModule_AddIntMacro(m, PF_DIVERT);
+#endif
+#ifdef AF_DIVERT
+    PyModule_AddIntMacro(m, AF_DIVERT);


Right, looking at the rest of the code it is expected to have both constants.

nirs · 2022-11-02T22:43:13Z

Misc/NEWS.d/next/Library/2022-09-03-09-24-02.gh-issue-96534.EU4Oxv.rst

@@ -0,0 +1 @@
+Support divert(4) on FreeBSD 14.


Maybe "Support divert(4) added in FreeBSD 14."?

koobs · 2022-11-03T01:50:12Z

For me, IPPROTO_DIVERT constant should be added, no?

Addressed in #96536 (comment), no longer awaiting changes

vstinner · 2022-11-03T12:45:44Z

I don't think there is any reason to add support for obsolete constant.

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.

glebius · 2022-11-03T16:21:12Z

I don't think there is any reason to add support for obsolete constant.
They are users running FreeBSD older than FreeBSD 14. Without IPPROTO_DIVERT exposed in Python, this kind of socket cannot be used in Python.

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. See this change.

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.

You can install this VM image from here https://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.

glebius · 2022-11-04T15:57:38Z

@vstinner @nirs @zooba Guys, who is actually handling this request?

vstinner · 2022-11-04T18:20:45Z

I'd really appreciate if this happens soon rather than wait for 14.0-RELEASE

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).

vstinner · 2022-11-04T18:21:27Z

The constant wasn't defined before and it stays undefined. The old style socket could actually be used, just using 258 literal.

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, and Python kinda promises to add support in 3.12 [1]. This reverts commit 322b5b7. [1] python/cpython#96536 (comment)

glebius · 2023-02-09T18:13:33Z

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, and Python kinda promises to add support in 3.12 [1]. This reverts commit 322b5b7. [1] python/cpython#96536 (comment)

vstinner

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.

vstinner · 2023-04-05T18:48:10Z

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).

glebius · 2023-04-10T23:22:30Z

@vstinner updated socket module documentation as you suggested. Let me know if I need anything else to be done to get this in.

glebius · 2023-04-20T17:27:02Z

@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.

vstinner

LGTM. Thanks for adding the documentation.

vstinner · 2023-05-04T14:57:41Z

Better late than never, I merged your PR. Thanks for your contribution.

koobs · 2023-05-05T03:24:09Z

Better late than never, I merged your PR. Thanks for your contribution.

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) ...

bedevere-bot added the awaiting review label Sep 3, 2022

vstinner requested changes Nov 2, 2022

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 2, 2022

nirs reviewed Nov 2, 2022

View reviewed changes

vstinner added the OS-freebsd label Nov 2, 2022

nirs reviewed Nov 2, 2022

View reviewed changes

bedevere-bot mentioned this pull request Nov 2, 2022

socketmodule: support FreeBSD divert(4) socket #96534

Closed

vstinner reviewed Apr 5, 2023

View reviewed changes

gh-96534: socketmodule: support FreeBSD divert(4) socket

1039ba2

vstinner enabled auto-merge (squash) May 4, 2023 14:56

vstinner approved these changes May 4, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels May 4, 2023

vstinner merged commit b17d32c into python:main May 4, 2023

bedevere-bot removed the awaiting merge label May 4, 2023

sobolevn mentioned this pull request Oct 24, 2023

socketmodule.c does not handle errors on module creation extension-modules #111253

Closed

gh-96534: socketmodule: support FreeBSD divert(4) socket #96536

gh-96534: socketmodule: support FreeBSD divert(4) socket #96536

Conversation

glebius commented Sep 3, 2022 • edited by bedevere-bot Loading

bedevere-bot commented Sep 3, 2022

cpython-cla-bot bot commented Sep 3, 2022 • edited Loading

bedevere-bot commented Sep 3, 2022

glebius commented Nov 2, 2022

zooba commented Nov 2, 2022

glebius commented Nov 2, 2022

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Nov 2, 2022

vstinner commented Nov 2, 2022

vstinner commented Nov 2, 2022

glebius commented Nov 2, 2022

glebius commented Nov 2, 2022

vstinner commented Nov 2, 2022

vstinner commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 2, 2022

koobs commented Nov 2, 2022

koobs commented Nov 2, 2022

vstinner commented Nov 2, 2022

glebius commented Nov 2, 2022

nirs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koobs commented Nov 3, 2022

vstinner commented Nov 3, 2022

glebius commented Nov 3, 2022

glebius commented Nov 4, 2022

vstinner commented Nov 4, 2022

vstinner commented Nov 4, 2022

glebius commented Feb 9, 2023 • edited Loading

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Apr 5, 2023

glebius commented Apr 10, 2023

glebius commented Apr 20, 2023

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented May 4, 2023

koobs commented May 5, 2023

glebius commented Sep 3, 2022 •

edited by bedevere-bot

Loading

cpython-cla-bot bot commented Sep 3, 2022 •

edited

Loading

glebius commented Feb 9, 2023 •

edited

Loading