[Python-checkins] bpo-40422: create a common _Py_closerange API (GH-19754)

[Kyle Evans]

https://github.com/python/cpython/commit/c230fde8475e4e5581e74a4235654d17ccf4cff8
commit: c230fde8475e4e5581e74a4235654d17ccf4cff8
branch: master
author: Kyle Evans <kevans91 at users.noreply.github.com>
committer: GitHub <noreply at github.com>
date: 2020-10-11T11:54:11-07:00
summary:

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

files:
A Misc/NEWS.d/next/C API/2020-10-10-14-05-24.bpo-40422.sh8IDY.rst
M Modules/_posixsubprocess.c
M Modules/posixmodule.c
M Modules/posixmodule.h

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 0000000000000..1b6d9e034b529
--- /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 5d1691ace4192..ed046fc5c1ba9 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 165625c9a670a..321eaec63c4a4 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 1e00562abc337..749833f71cd4d 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