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

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

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

vstinner

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

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

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

FreeBSD 14 divert manual page

vstinner

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

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

vstinner

cc @koobs

koobs

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

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

cc @Jehops @emaste

glebius

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.

koobs

For me, IPPROTO_DIVERT constant should be added, no?

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

vstinner

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

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

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

vstinner

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

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

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

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

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

glebius

@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

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

koobs

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 and removed 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

glebius mentioned this pull request Dec 19, 2025

socketmodule: FreeBSD PF_DIVERT is missing in getsockaddrlen() #142991

Closed

Conversation

glebius commented Sep 3, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Sep 3, 2022

Uh oh!

ghost commented Sep 3, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Sep 3, 2022

Uh oh!

glebius commented Nov 2, 2022

Uh oh!

zooba commented Nov 2, 2022

Uh oh!

glebius commented Nov 2, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 2, 2022

Uh oh!

vstinner commented Nov 2, 2022

Uh oh!

vstinner commented Nov 2, 2022

Uh oh!

glebius commented Nov 2, 2022

Uh oh!

glebius commented Nov 2, 2022

Uh oh!

vstinner commented Nov 2, 2022

Uh oh!

vstinner commented Nov 2, 2022

Uh oh!

vstinner commented Nov 2, 2022

Uh oh!

koobs commented Nov 2, 2022

Uh oh!

koobs commented Nov 2, 2022

Uh oh!

vstinner commented Nov 2, 2022

Uh oh!

glebius commented Nov 2, 2022

Uh oh!

nirs left a comment

Choose a reason for hiding this comment

Uh oh!

koobs commented Nov 3, 2022

Uh oh!

vstinner commented Nov 3, 2022

Uh oh!

glebius commented Nov 3, 2022

Uh oh!

glebius commented Nov 4, 2022

Uh oh!

vstinner commented Nov 4, 2022

Uh oh!

vstinner commented Nov 4, 2022

Uh oh!

glebius commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Apr 5, 2023

Uh oh!

glebius commented Apr 10, 2023

Uh oh!

glebius commented Apr 20, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 4, 2023

Uh oh!

koobs commented May 5, 2023

Uh oh!

Reviewers

Assignees

glebius commented Sep 3, 2022 •

edited by bedevere-bot

Loading

ghost commented Sep 3, 2022 •

edited by ghost

Loading

glebius commented Feb 9, 2023 •

edited

Loading