From c230fde8475e4e5581e74a4235654d17ccf4cff8 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Sun, 11 Oct 2020 13:54:11 -0500 Subject: [PATCH] bpo-40422: create a common _Py_closerange API (GH-19754) 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 --- .../2020-10-10-14-05-24.bpo-40422.sh8IDY.rst | 1 + Modules/_posixsubprocess.c | 15 +--- Modules/posixmodule.c | 70 ++++++++++++------- Modules/posixmodule.h | 2 + 4 files changed, 51 insertions(+), 37 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2020-10-10-14-05-24.bpo-40422.sh8IDY.rst diff --git a/Misc/NEWS.d/next/C API/2020-10-10-14-05-24.bpo-40422.sh8IDY.rst b/Misc/NEWS.d/next/C API/2020-10-10-14-05-24.bpo-40422.sh8IDY.rst new file mode 100644 index 00000000000..1b6d9e034b5 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2020-10-10-14-05-24.bpo-40422.sh8IDY.rst @@ -0,0 +1 @@ +Add `_Py_closerange` function to provide performant closing of a range of file descriptors. \ No newline at end of file diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 5d1691ace41..ed046fc5c1b 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -250,7 +250,6 @@ _close_fds_by_brute_force(long start_fd, PyObject *py_fds_to_keep) long end_fd = safe_get_max_fd(); Py_ssize_t num_fds_to_keep = PyTuple_GET_SIZE(py_fds_to_keep); Py_ssize_t keep_seq_idx; - int fd_num; /* As py_fds_to_keep is sorted we can loop through the list closing * fds in between any in the keep list falling within our range. */ for (keep_seq_idx = 0; keep_seq_idx < num_fds_to_keep; ++keep_seq_idx) { @@ -258,21 +257,11 @@ _close_fds_by_brute_force(long start_fd, PyObject *py_fds_to_keep) int keep_fd = PyLong_AsLong(py_keep_fd); if (keep_fd < start_fd) continue; - for (fd_num = start_fd; fd_num < keep_fd; ++fd_num) { - close(fd_num); - } + _Py_closerange(start_fd, keep_fd - 1); start_fd = keep_fd + 1; } if (start_fd <= end_fd) { -#if defined(__FreeBSD__) - /* Any errors encountered while closing file descriptors are ignored */ - closefrom(start_fd); -#else - for (fd_num = start_fd; fd_num < end_fd; ++fd_num) { - /* Ignore errors */ - (void)close(fd_num); - } -#endif + _Py_closerange(start_fd, end_fd); } } diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 165625c9a67..321eaec63c4 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -8740,8 +8740,23 @@ os_close_impl(PyObject *module, int fd) Py_RETURN_NONE; } +/* Our selection logic for which function to use is as follows: + * 1. If closefrom(2) is available, we'll attempt to use that next if we're + * closing up to sysconf(_SC_OPEN_MAX). + * 1a. Fallback to fdwalk(3) if we're not closing up to sysconf(_SC_OPEN_MAX), + * as that will be more performant if the range happens to have any chunk of + * non-opened fd in the middle. + * 1b. If fdwalk(3) isn't available, just do a plain close(2) loop. + */ +#ifdef __FreeBSD__ +#define USE_CLOSEFROM +#endif /* __FreeBSD__ */ #ifdef HAVE_FDWALK +#define USE_FDWALK +#endif /* HAVE_FDWALK */ + +#ifdef USE_FDWALK static int _fdwalk_close_func(void *lohi, int fd) { @@ -8757,7 +8772,36 @@ _fdwalk_close_func(void *lohi, int fd) } return 0; } -#endif /* HAVE_FDWALK */ +#endif /* USE_FDWALK */ + +/* Closes all file descriptors in [first, last], ignoring errors. */ +void +_Py_closerange(int first, int last) +{ + first = Py_MAX(first, 0); +#ifdef USE_CLOSEFROM + if (last >= sysconf(_SC_OPEN_MAX)) { + /* Any errors encountered while closing file descriptors are ignored */ + closefrom(first); + } + else +#endif /* USE_CLOSEFROM */ +#ifdef USE_FDWALK + { + int lohi[2]; + lohi[0] = first; + lohi[1] = last + 1; + fdwalk(_fdwalk_close_func, lohi); + } +#else + { + for (int i = first; i <= last; i++) { + /* Ignore errors */ + (void)close(i); + } + } +#endif /* USE_FDWALK */ +} /*[clinic input] os.closerange @@ -8773,31 +8817,9 @@ static PyObject * os_closerange_impl(PyObject *module, int fd_low, int fd_high) /*[clinic end generated code: output=0ce5c20fcda681c2 input=5855a3d053ebd4ec]*/ { -#ifdef HAVE_FDWALK - int lohi[2]; -#endif Py_BEGIN_ALLOW_THREADS _Py_BEGIN_SUPPRESS_IPH -#ifdef HAVE_FDWALK - lohi[0] = Py_MAX(fd_low, 0); - lohi[1] = fd_high; - fdwalk(_fdwalk_close_func, lohi); -#else - fd_low = Py_MAX(fd_low, 0); -#ifdef __FreeBSD__ - if (fd_high >= sysconf(_SC_OPEN_MAX)) { - /* Any errors encountered while closing file descriptors are ignored */ - closefrom(fd_low); - } - else -#endif - { - for (int i = fd_low; i < fd_high; i++) { - /* Ignore errors */ - (void)close(i); - } - } -#endif + _Py_closerange(fd_low, fd_high - 1); _Py_END_SUPPRESS_IPH Py_END_ALLOW_THREADS Py_RETURN_NONE; diff --git a/Modules/posixmodule.h b/Modules/posixmodule.h index 1e00562abc3..749833f71cd 100644 --- a/Modules/posixmodule.h +++ b/Modules/posixmodule.h @@ -28,6 +28,8 @@ PyAPI_FUNC(int) _Py_Sigset_Converter(PyObject *, void *); #endif /* HAVE_SIGSET_T */ #endif /* Py_LIMITED_API */ +PyAPI_FUNC(void) _Py_closerange(int first, int last); + #ifdef __cplusplus } #endif