[PATCH v7 0/5] qemu/osdep: add a qemu_close_all_open_fd() helper

[PATCH v7 0/5] qemu/osdep: add a qemu_close_all_open_fd() helper

[Clément Léger]

Since commit 03e471c41d8b ("qemu_init: increase NOFILE soft limit on
POSIX"), the maximum number of file descriptors that can be opened are
raised to nofile.rlim_max. On recent debian distro, this yield a maximum
of 1073741816 file descriptors. Now, when forking to start
qemu-bridge-helper, this actually calls close() on the full possible file
descriptor range (more precisely [3 - sysconf(_SC_OPEN_MAX)]) which
takes a considerable amount of time. In order to reduce that time,
factorize existing code to close all open files descriptors in a new
qemu_close_all_open_fd() function. This function uses various methods
to close all the open file descriptors ranging from the most efficient
one to the least one. It also accepts an ordered array of file
descriptors that should not be closed since this is required by the
callers that calls it after forking. Since this function is not used
for Win32, do not implement it to force an error at link time if used.

---

v7:
 - Passed open_max to qemu_close_all_open_fd() subfunctions
 - Remove extra whitespace
 - Reduce some variable scopes
 - v7: https://lore.kernel.org/qemu-devel/20240730122437.1749603-1-cleger@rivosinc.com/

v6:
 - Split patch in multiple commits
 - Drop Richard Henderson Reviewed-by since there was a lot of
   modifications
 - Remove useless #ifdef LINUX in qemu_close_all_open_fd_proc()
 - v5: https://lore.kernel.org/qemu-devel/20240726075502.4054284-1-cleger@rivosinc.com/

v5:
 - Move qemu_close_all_open_fd() to oslib-posix.c since it does not
   compile on windows and is not even used on it.
 - v4: https://lore.kernel.org/qemu-devel/20240717124534.1200735-1-cleger@rivosinc.com/

v4:
 - Add a comment saying that qemu_close_all_open_fd() can take a NULL skip
   array and nskip == 0
 - Added an assert in qemu_close_all_open_fd() to check for skip/nskip
   parameters
 - Fix spurious tabs instead of spaces
 - Applied checkpatch
 - v3: https://lore.kernel.org/qemu-devel/20240716144006.6571-1-cleger@rivosinc.com/

v3:
 - Use STD*_FILENO defines instead of raw values
 - Fix indentation of close_all_fds_after_fork()
 - Check for nksip in fallback code
 - Check for path starting with a '.' in qemu_close_all_open_fd_proc()
 - Use unsigned for cur_skip
 - Move ifdefs inside close_fds functions rather than redefining them
 - Remove uneeded 'if(nskip)' test
 - Add comments to close_range version
 - Reduce range of skip fd as we find them in
 - v2: https://lore.kernel.org/qemu-devel/20240618111704.63092-1-cleger@rivosinc.com/

v2:
 - Factorize async_teardown.c close_fds implementation as well as tap.c ones
 - Apply checkpatch
 - v1: https://lore.kernel.org/qemu-devel/20240617162520.4045016-1-cleger@rivosinc.com/

Clément Léger (5):
  qemu/osdep: Move close_all_open_fds() to oslib-posix
  qemu/osdep: Split qemu_close_all_open_fd() and add fallback
  net/tap: Factorize fd closing after forking
  qemu/osdep: Add excluded fd parameter to qemu_close_all_open_fd()
  net/tap: Use qemu_close_all_open_fd()

 include/qemu/osdep.h    |  11 ++++
 net/tap.c               |  34 +++++-----
 system/async-teardown.c |  37 +----------
 util/oslib-posix.c      | 133 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 165 insertions(+), 50 deletions(-)

-- 
2.45.2

[PATCH v7 1/5] qemu/osdep: Move close_all_open_fds() to oslib-posix

[Clément Léger]

Move close_all_open_fds() in oslib-posix, rename it
qemu_close_all_open_fds() and export it.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/osdep.h    |  7 +++++++
 system/async-teardown.c | 37 +------------------------------------
 util/oslib-posix.c      | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 191916f38e..5cd8517380 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -757,6 +757,13 @@ static inline void qemu_reset_optind(void)
 
 int qemu_fdatasync(int fd);
 
+/**
+ * qemu_close_all_open_fd:
+ *
+ * Close all open file descriptors
+ */
+void qemu_close_all_open_fd(void);
+
 /**
  * Sync changes made to the memory mapped file back to the backing
  * storage. For POSIX compliant systems this will fallback
diff --git a/system/async-teardown.c b/system/async-teardown.c
index 396963c091..edf49e1007 100644
--- a/system/async-teardown.c
+++ b/system/async-teardown.c
@@ -26,40 +26,6 @@
 
 static pid_t the_ppid;
 
-/*
- * Close all open file descriptors.
- */
-static void close_all_open_fd(void)
-{
-    struct dirent *de;
-    int fd, dfd;
-    DIR *dir;
-
-#ifdef CONFIG_CLOSE_RANGE
-    int r = close_range(0, ~0U, 0);
-    if (!r) {
-        /* Success, no need to try other ways. */
-        return;
-    }
-#endif
-
-    dir = opendir("/proc/self/fd");
-    if (!dir) {
-        /* If /proc is not mounted, there is nothing that can be done. */
-        return;
-    }
-    /* Avoid closing the directory. */
-    dfd = dirfd(dir);
-
-    for (de = readdir(dir); de; de = readdir(dir)) {
-        fd = atoi(de->d_name);
-        if (fd != dfd) {
-            close(fd);
-        }
-    }
-    closedir(dir);
-}
-
 static void hup_handler(int signal)
 {
     /* Check every second if this process has been reparented. */
@@ -85,9 +51,8 @@ static int async_teardown_fn(void *arg)
     /*
      * Close all file descriptors that might have been inherited from the
      * main qemu process when doing clone, needed to make libvirt happy.
-     * Not using close_range for increased compatibility with older kernels.
      */
-    close_all_open_fd();
+    qemu_close_all_open_fd();
 
     /* Set up a handler for SIGHUP and unblock SIGHUP. */
     sigaction(SIGHUP, &sa, NULL);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index b090fe0eed..1e867efa47 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -807,3 +807,37 @@ int qemu_msync(void *addr, size_t length, int fd)
 
     return msync(addr, length, MS_SYNC);
 }
+
+/*
+ * Close all open file descriptors.
+ */
+void qemu_close_all_open_fd(void)
+{
+    struct dirent *de;
+    int fd, dfd;
+    DIR *dir;
+
+#ifdef CONFIG_CLOSE_RANGE
+    int r = close_range(0, ~0U, 0);
+    if (!r) {
+        /* Success, no need to try other ways. */
+        return;
+    }
+#endif
+
+    dir = opendir("/proc/self/fd");
+    if (!dir) {
+        /* If /proc is not mounted, there is nothing that can be done. */
+        return;
+    }
+    /* Avoid closing the directory. */
+    dfd = dirfd(dir);
+
+    for (de = readdir(dir); de; de = readdir(dir)) {
+        fd = atoi(de->d_name);
+        if (fd != dfd) {
+            close(fd);
+        }
+    }
+    closedir(dir);
+}
-- 
2.45.2

[PATCH v7 2/5] qemu/osdep: Split qemu_close_all_open_fd() and add fallback

[Clément Léger]

In order to make it cleaner, split qemu_close_all_open_fd() logic into
multiple subfunctions (close with close_range(), with /proc/self/fd and
fallback).

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 util/oslib-posix.c | 50 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 13 deletions(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 1e867efa47..9b79fc7cff 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -808,27 +808,16 @@ int qemu_msync(void *addr, size_t length, int fd)
     return msync(addr, length, MS_SYNC);
 }
 
-/*
- * Close all open file descriptors.
- */
-void qemu_close_all_open_fd(void)
+static bool qemu_close_all_open_fd_proc(void)
 {
     struct dirent *de;
     int fd, dfd;
     DIR *dir;
 
-#ifdef CONFIG_CLOSE_RANGE
-    int r = close_range(0, ~0U, 0);
-    if (!r) {
-        /* Success, no need to try other ways. */
-        return;
-    }
-#endif
-
     dir = opendir("/proc/self/fd");
     if (!dir) {
         /* If /proc is not mounted, there is nothing that can be done. */
-        return;
+        return false;
     }
     /* Avoid closing the directory. */
     dfd = dirfd(dir);
@@ -840,4 +829,39 @@ void qemu_close_all_open_fd(void)
         }
     }
     closedir(dir);
+
+    return true;
+}
+
+static bool qemu_close_all_open_fd_close_range(void)
+{
+#ifdef CONFIG_CLOSE_RANGE
+    int r = close_range(0, ~0U, 0);
+    if (!r) {
+        /* Success, no need to try other ways. */
+        return true;
+    }
+#endif
+    return false;
+}
+
+static void qemu_close_all_open_fd_fallback(void)
+{
+    int open_max = sysconf(_SC_OPEN_MAX), i;
+
+    /* Fallback */
+    for (i = 0; i < open_max; i++) {
+        close(i);
+    }
+}
+
+/*
+ * Close all open file descriptors.
+ */
+void qemu_close_all_open_fd(void)
+{
+    if (!qemu_close_all_open_fd_close_range() &&
+        !qemu_close_all_open_fd_proc()) {
+        qemu_close_all_open_fd_fallback();
+    }
 }
-- 
2.45.2

Re: [PATCH v7 2/5] qemu/osdep: Split qemu_close_all_open_fd() and add fallback

[Richard Henderson]

On 7/31/24 18:48, Clément Léger wrote:
> In order to make it cleaner, split qemu_close_all_open_fd() logic into
> multiple subfunctions (close with close_range(), with /proc/self/fd and
> fallback).
> 
> Signed-off-by: Clément Léger<cleger@rivosinc.com>
> ---
>   util/oslib-posix.c | 50 ++++++++++++++++++++++++++++++++++------------
>   1 file changed, 37 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v7 3/5] net/tap: Factorize fd closing after forking

[Clément Léger]

The same code is used twice to actually close all open file descriptors
after forking. Factorize it in a single place.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 net/tap.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 51f7aec39d..7b2d5d5703 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -385,6 +385,17 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
     return s;
 }
 
+static void close_all_fds_after_fork(int excluded_fd)
+{
+    int open_max = sysconf(_SC_OPEN_MAX), i;
+
+    for (i = 3; i < open_max; i++) {
+        if (i != excluded_fd) {
+            close(i);
+        }
+    }
+}
+
 static void launch_script(const char *setup_script, const char *ifname,
                           int fd, Error **errp)
 {
@@ -400,13 +411,7 @@ static void launch_script(const char *setup_script, const char *ifname,
         return;
     }
     if (pid == 0) {
-        int open_max = sysconf(_SC_OPEN_MAX), i;
-
-        for (i = 3; i < open_max; i++) {
-            if (i != fd) {
-                close(i);
-            }
-        }
+        close_all_fds_after_fork(fd);
         parg = args;
         *parg++ = (char *)setup_script;
         *parg++ = (char *)ifname;
@@ -490,17 +495,11 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
         return -1;
     }
     if (pid == 0) {
-        int open_max = sysconf(_SC_OPEN_MAX), i;
         char *fd_buf = NULL;
         char *br_buf = NULL;
         char *helper_cmd = NULL;
 
-        for (i = 3; i < open_max; i++) {
-            if (i != sv[1]) {
-                close(i);
-            }
-        }
-
+        close_all_fds_after_fork(sv[1]);
         fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
 
         if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
-- 
2.45.2

[PATCH v7 4/5] qemu/osdep: Add excluded fd parameter to qemu_close_all_open_fd()

[Clément Léger]

In order for this function to be usable by tap.c code, add a list of
file descriptors that should not be closed.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 include/qemu/osdep.h    |   8 ++-
 system/async-teardown.c |   2 +-
 util/oslib-posix.c      | 107 ++++++++++++++++++++++++++++++++++------
 3 files changed, 98 insertions(+), 19 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 5cd8517380..0bf6f0a356 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -760,9 +760,13 @@ int qemu_fdatasync(int fd);
 /**
  * qemu_close_all_open_fd:
  *
- * Close all open file descriptors
+ * Close all open file descriptors except the ones supplied in the @skip array
+ *
+ * @skip: ordered array of distinct file descriptors that should not be closed
+ *        if any, or NULL.
+ * @nskip: number of entries in the @skip array or 0 if @skip is NULL.
  */
-void qemu_close_all_open_fd(void);
+void qemu_close_all_open_fd(const int *skip, unsigned int nskip);
 
 /**
  * Sync changes made to the memory mapped file back to the backing
diff --git a/system/async-teardown.c b/system/async-teardown.c
index edf49e1007..9148ee8d04 100644
--- a/system/async-teardown.c
+++ b/system/async-teardown.c
@@ -52,7 +52,7 @@ static int async_teardown_fn(void *arg)
      * Close all file descriptors that might have been inherited from the
      * main qemu process when doing clone, needed to make libvirt happy.
      */
-    qemu_close_all_open_fd();
+    qemu_close_all_open_fd(NULL, 0);
 
     /* Set up a handler for SIGHUP and unblock SIGHUP. */
     sigaction(SIGHUP, &sa, NULL);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 9b79fc7cff..7583192192 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -808,11 +808,12 @@ int qemu_msync(void *addr, size_t length, int fd)
     return msync(addr, length, MS_SYNC);
 }
 
-static bool qemu_close_all_open_fd_proc(void)
+static bool qemu_close_all_open_fd_proc(const int *skip, unsigned int nskip)
 {
     struct dirent *de;
     int fd, dfd;
     DIR *dir;
+    unsigned int skip_start = 0, skip_end = nskip;
 
     dir = opendir("/proc/self/fd");
     if (!dir) {
@@ -823,8 +824,34 @@ static bool qemu_close_all_open_fd_proc(void)
     dfd = dirfd(dir);
 
     for (de = readdir(dir); de; de = readdir(dir)) {
+        bool close_fd = true;
+
+        if (de->d_name[0] == '.') {
+            continue;
+        }
         fd = atoi(de->d_name);
-        if (fd != dfd) {
+        if (fd == dfd) {
+            close_fd = false;
+            continue;
+        }
+
+        for (unsigned int i = skip_start; i < skip_end; i++) {
+            if (fd < skip[i]) {
+                /* We are below the next skipped fd, break */
+                break;
+            } else if (fd == skip[i]) {
+                close_fd = false;
+                /* Restrict the range as we found fds matching start/end */
+                if (i == skip_start) {
+                    skip_start++;
+                } else if (i == skip_end) {
+                    skip_end--;
+                }
+                break;
+            }
+        }
+
+        if (close_fd) {
             close(fd);
         }
     }
@@ -833,24 +860,68 @@ static bool qemu_close_all_open_fd_proc(void)
     return true;
 }
 
-static bool qemu_close_all_open_fd_close_range(void)
+static bool qemu_close_all_open_fd_close_range(const int *skip,
+                                               unsigned int nskip,
+                                               int open_max)
 {
 #ifdef CONFIG_CLOSE_RANGE
-    int r = close_range(0, ~0U, 0);
-    if (!r) {
-        /* Success, no need to try other ways. */
-        return true;
-    }
-#endif
+    int first = 0, last = open_max;
+    unsigned int cur_skip = 0;
+    int ret;
+
+    do {
+        /* Find the start boundary of the range to close */
+        while (cur_skip < nskip && first == skip[cur_skip]) {
+            cur_skip++;
+            first++;
+        }
+
+        /* Find the upper boundary of the range to close */
+        if (cur_skip < nskip) {
+            last = skip[cur_skip] - 1;
+        }
+        /*
+         * Adjust the maximum fd to close if it's above what the system
+         * supports
+         */
+        if (last > open_max) {
+            last = open_max;
+            /*
+             * We can directly skip the remaining skip fds since the current
+             * one is already above the maximum supported one.
+             */
+            cur_skip = nskip;
+        }
+        /* If last was adjusted, we might be > first, bail out */
+        if (first > last) {
+            break;
+        }
+
+        ret = close_range(first, last, 0);
+        if (ret < 0) {
+            return false;
+        }
+        first = last + 1;
+        last = open_max;
+    } while (cur_skip < nskip);
+
+    return true;
+#else
     return false;
+#endif
 }
 
-static void qemu_close_all_open_fd_fallback(void)
+static void qemu_close_all_open_fd_fallback(const int *skip, unsigned int nskip,
+                                            int open_max)
 {
-    int open_max = sysconf(_SC_OPEN_MAX), i;
+    unsigned int cur_skip = 0;
 
     /* Fallback */
-    for (i = 0; i < open_max; i++) {
+    for (int i = 0; i < open_max; i++) {
+        if (cur_skip < nskip && i == skip[cur_skip]) {
+            cur_skip++;
+            continue;
+        }
         close(i);
     }
 }
@@ -858,10 +929,14 @@ static void qemu_close_all_open_fd_fallback(void)
 /*
  * Close all open file descriptors.
  */
-void qemu_close_all_open_fd(void)
+void qemu_close_all_open_fd(const int *skip, unsigned int nskip)
 {
-    if (!qemu_close_all_open_fd_close_range() &&
-        !qemu_close_all_open_fd_proc()) {
-        qemu_close_all_open_fd_fallback();
+    int open_max = sysconf(_SC_OPEN_MAX) - 1;
+
+    assert(skip != NULL || nskip == 0);
+
+    if (!qemu_close_all_open_fd_close_range(skip, nskip, open_max) &&
+        !qemu_close_all_open_fd_proc(skip, nskip)) {
+        qemu_close_all_open_fd_fallback(skip, nskip, open_max);
     }
 }
-- 
2.45.2

Re: [PATCH v7 4/5] qemu/osdep: Add excluded fd parameter to qemu_close_all_open_fd()

[Richard Henderson]

On 7/31/24 18:48, Clément Léger wrote:
> @@ -823,8 +824,34 @@ static bool qemu_close_all_open_fd_proc(void)
>       dfd = dirfd(dir);
>   
>       for (de = readdir(dir); de; de = readdir(dir)) {
> +        bool close_fd = true;
> +
> +        if (de->d_name[0] == '.') {
> +            continue;
> +        }
>           fd = atoi(de->d_name);
> -        if (fd != dfd) {
> +        if (fd == dfd) {
> +            close_fd = false;
> +            continue;
> +        }

Assignment to close_fd not used here.

> +
> +        for (unsigned int i = skip_start; i < skip_end; i++) {
> +            if (fd < skip[i]) {
> +                /* We are below the next skipped fd, break */
> +                break;
> +            } else if (fd == skip[i]) {
> +                close_fd = false;
> +                /* Restrict the range as we found fds matching start/end */
> +                if (i == skip_start) {
> +                    skip_start++;
> +                } else if (i == skip_end) {
> +                    skip_end--;
> +                }
> +                break;
> +            }
> +        }
> +
> +        if (close_fd) {
>               close(fd);
>           }
>       }
> @@ -833,24 +860,68 @@ static bool qemu_close_all_open_fd_proc(void)
>       return true;
>   }
>   
> -static bool qemu_close_all_open_fd_close_range(void)
> +static bool qemu_close_all_open_fd_close_range(const int *skip,
> +                                               unsigned int nskip,
> +                                               int open_max)
>   {
>   #ifdef CONFIG_CLOSE_RANGE
> -    int r = close_range(0, ~0U, 0);
> -    if (!r) {
> -        /* Success, no need to try other ways. */
> -        return true;
> -    }
> -#endif
> +    int first = 0, last = open_max;

If this were really _SC_OPEN_MAX, this would be off-by-one.

> -static void qemu_close_all_open_fd_fallback(void)
> +static void qemu_close_all_open_fd_fallback(const int *skip, unsigned int nskip,
> +                                            int open_max)
>   {
> -    int open_max = sysconf(_SC_OPEN_MAX), i;
> +    unsigned int cur_skip = 0;
>   
>       /* Fallback */
> -    for (i = 0; i < open_max; i++) {
> +    for (int i = 0; i < open_max; i++) {

Because this isn't really _SC_OPEN_MAX, this *is* off-by-one.

> -void qemu_close_all_open_fd(void)
> +void qemu_close_all_open_fd(const int *skip, unsigned int nskip)
>   {
> -    if (!qemu_close_all_open_fd_close_range() &&
> -        !qemu_close_all_open_fd_proc()) {
> -        qemu_close_all_open_fd_fallback();
> +    int open_max = sysconf(_SC_OPEN_MAX) - 1;

Better to move the -1 into qemu_close_all_open_fd_close_range where it's actually needed.


r~

Re: [PATCH v7 4/5] qemu/osdep: Add excluded fd parameter to qemu_close_all_open_fd()

[Clément Léger]

On 31/07/2024 23:56, Richard Henderson wrote:
> On 7/31/24 18:48, Clément Léger wrote:
>> @@ -823,8 +824,34 @@ static bool qemu_close_all_open_fd_proc(void)
>>       dfd = dirfd(dir);
>>         for (de = readdir(dir); de; de = readdir(dir)) {
>> +        bool close_fd = true;
>> +
>> +        if (de->d_name[0] == '.') {
>> +            continue;
>> +        }
>>           fd = atoi(de->d_name);
>> -        if (fd != dfd) {
>> +        if (fd == dfd) {
>> +            close_fd = false;
>> +            continue;
>> +        }
> 
> Assignment to close_fd not used here.
> 
>> +
>> +        for (unsigned int i = skip_start; i < skip_end; i++) {
>> +            if (fd < skip[i]) {
>> +                /* We are below the next skipped fd, break */
>> +                break;
>> +            } else if (fd == skip[i]) {
>> +                close_fd = false;
>> +                /* Restrict the range as we found fds matching
>> start/end */
>> +                if (i == skip_start) {
>> +                    skip_start++;
>> +                } else if (i == skip_end) {
>> +                    skip_end--;
>> +                }
>> +                break;
>> +            }
>> +        }
>> +
>> +        if (close_fd) {
>>               close(fd);
>>           }
>>       }
>> @@ -833,24 +860,68 @@ static bool qemu_close_all_open_fd_proc(void)
>>       return true;
>>   }
>>   -static bool qemu_close_all_open_fd_close_range(void)
>> +static bool qemu_close_all_open_fd_close_range(const int *skip,
>> +                                               unsigned int nskip,
>> +                                               int open_max)
>>   {
>>   #ifdef CONFIG_CLOSE_RANGE
>> -    int r = close_range(0, ~0U, 0);
>> -    if (!r) {
>> -        /* Success, no need to try other ways. */
>> -        return true;
>> -    }
>> -#endif
>> +    int first = 0, last = open_max;
> 
> If this were really _SC_OPEN_MAX, this would be off-by-one.
> 
>> -static void qemu_close_all_open_fd_fallback(void)
>> +static void qemu_close_all_open_fd_fallback(const int *skip, unsigned
>> int nskip,
>> +                                            int open_max)
>>   {
>> -    int open_max = sysconf(_SC_OPEN_MAX), i;
>> +    unsigned int cur_skip = 0;
>>         /* Fallback */
>> -    for (i = 0; i < open_max; i++) {
>> +    for (int i = 0; i < open_max; i++) {
> 
> Because this isn't really _SC_OPEN_MAX, this *is* off-by-one.
> 
>> -void qemu_close_all_open_fd(void)
>> +void qemu_close_all_open_fd(const int *skip, unsigned int nskip)
>>   {
>> -    if (!qemu_close_all_open_fd_close_range() &&
>> -        !qemu_close_all_open_fd_proc()) {
>> -        qemu_close_all_open_fd_fallback();
>> +    int open_max = sysconf(_SC_OPEN_MAX) - 1;
> 
> Better to move the -1 into qemu_close_all_open_fd_close_range where it's
> actually needed.

Arg, I had it right in the previous version but messed up with the
SC_OPEN_MAX factorization.

I'll fix that,

Thanks,

CLément

> 
> 
> r~

[PATCH v7 5/5] net/tap: Use qemu_close_all_open_fd()

[Clément Léger]

Instead of using a slow implementation to close all open fd after
forking, use qemu_close_all_open_fd().

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 net/tap.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 7b2d5d5703..3f90022c0b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -387,13 +387,20 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
 
 static void close_all_fds_after_fork(int excluded_fd)
 {
-    int open_max = sysconf(_SC_OPEN_MAX), i;
+    const int skip_fd[] = {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO,
+                           excluded_fd};
+    unsigned int nskip = ARRAY_SIZE(skip_fd);
 
-    for (i = 3; i < open_max; i++) {
-        if (i != excluded_fd) {
-            close(i);
-        }
+    /*
+     * skip_fd must be an ordered array of distinct fds, exclude
+     * excluded_fd if already included in the [STDIN_FILENO - STDERR_FILENO]
+     * range
+     */
+    if (excluded_fd <= STDERR_FILENO) {
+        nskip--;
     }
+
+    qemu_close_all_open_fd(skip_fd, nskip);
 }
 
 static void launch_script(const char *setup_script, const char *ifname,
-- 
2.45.2