Skip to content
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

bpo-40422: create a common _Py_closerange API #19754

Merged
merged 7 commits into from
Oct 11, 2020

Conversation

kevans91
Copy link
Contributor

@kevans91 kevans91 commented Apr 28, 2020

Such an API can be used both for os.closerange and subprocess. For the latter, this yields potential improvement for platforms that have fdwalk but wouldn't have used it there. This will prove even more beneficial later for platforms that have close_range(2), as the new API will prefer that over all else if it's available.

The new API is structured to look more like close_range(2), closing from [start, end] rather than the [low, high) of os.closerange().

https://bugs.python.org/issue40422

Automerge-Triggered-By: @gpshead

@kevans91 kevans91 requested a review from gpshead as a code owner April 28, 2020 13:43
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@kevans91

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

In a later commit, subprocess will be modified to use this instead of
hand-rolling it.
_Py_closerange will be more efficient than what was done before in many
cases, as it includes the logic to use fdwalk(3) if available that
_close_fds_by_brute_force didn't previously have. It will also oftentimes be
more efficient as _Py_closerange will later be enhanced to use
close_range(2) if it's available.
@kevans91
Copy link
Contributor Author

kevans91 commented Aug 5, 2020

Anything I can do to help this along?

Also, not sure if this is is necessarily worth adding a news entry, but I can do that if requested.

@kevans91
Copy link
Contributor Author

kevans91 commented Oct 9, 2020

Ping... same question as last time, anything I can do to help this along?

Modules/_posixsubprocess.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

FYI - Linux appears to be gaining a close_range system call this year but writing the code to determine if that syscall is available on the running kernel and use it belongs in a separate PR.
http://lkml.iu.edu/hypermail/linux/kernel/2008.0/02649.html

@kevans91
Copy link
Contributor Author

thanks!

FYI - Linux appears to be gaining a close_range system call this year but writing the code to determine if that syscall is available on the running kernel and use it belongs in a separate PR.
http://lkml.iu.edu/hypermail/linux/kernel/2008.0/02649.html

Indeed, thanks for taking a look!

I actually have a branch queued up with the rest in the pipeline that was awaiting this review -- I've split it up into configure script change and then preferring it in _Py_closerange: https://github.com/kevans91/cpython/commits/close_range

@miss-islington
Copy link
Contributor

@kevans91: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit c230fde into python:master Oct 11, 2020
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
Such an API can be used both for os.closerange and subprocess. For the latter, this yields potential improvement for platforms that have fdwalk but wouldn't have used it there. This will prove even more beneficial later for platforms that have close_range(2), as the new API will prefer that over all else if it's available.

The new API is structured to look more like close_range(2), closing from [start, end] rather than the [low, high) of os.closerange().

Automerge-Triggered-By: @gpshead
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Such an API can be used both for os.closerange and subprocess. For the latter, this yields potential improvement for platforms that have fdwalk but wouldn't have used it there. This will prove even more beneficial later for platforms that have close_range(2), as the new API will prefer that over all else if it's available.

The new API is structured to look more like close_range(2), closing from [start, end] rather than the [low, high) of os.closerange().

Automerge-Triggered-By: @gpshead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants