Skip to content

bpo-40422: create a common _Py_closerange API#19754

Merged
miss-islington merged 7 commits into
python:masterfrom
kevans91:py_closerange
Oct 11, 2020
Merged

bpo-40422: create a common _Py_closerange API#19754
miss-islington merged 7 commits into
python:masterfrom
kevans91:py_closerange

Conversation

@kevans91

@kevans91 kevans91 commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown

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

kevans91 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

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

kevans91 commented Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

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

Comment thread Modules/_posixsubprocess.c Outdated
Comment thread Modules/posixmodule.c Outdated

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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