On Fri, Jan 26, 2024 at 08:44:13AM +0100, Laurent Vivier wrote: > Le 25/01/2024 à 23:29, Michael Tokarev a écrit : > > Initially in async-teardown.c, but the same construct is used > > elsewhere too. > > > > Signed-off-by: Michael Tokarev <[email protected]> > > --- > > include/sysemu/os-posix.h | 1 + > > system/async-teardown.c | 37 +------------------------------------ > > util/oslib-posix.c | 36 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 38 insertions(+), 36 deletions(-) > > > > diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h > > index dff32ae185..4c91d03f44 100644 > > --- a/include/sysemu/os-posix.h > > +++ b/include/sysemu/os-posix.h > > @@ -53,6 +53,7 @@ bool os_set_runas(const char *user_id); > > void os_set_chroot(const char *path); > > void os_setup_post(void); > > int os_mlock(void); > > +void os_close_all_open_fd(int minfd); > > /** > > * qemu_alloc_stack: > > diff --git a/system/async-teardown.c b/system/async-teardown.c > > index 396963c091..41d3d94935 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(); > > + os_close_all_open_fd(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 7c297003b9..09d0ce4da6 100644 > > --- a/util/oslib-posix.c > > +++ b/util/oslib-posix.c > > @@ -27,6 +27,7 @@ > > */ > > #include "qemu/osdep.h" > > +#include <dirent.h> > > #include <termios.h> > > #include <glib/gprintf.h> > > @@ -106,6 +107,41 @@ int qemu_get_thread_id(void) > > #endif > > } > > +/* > > + * Close all open file descriptors starting with minfd and up. > > + * Not using close_range for increased compatibility with older kernels. > > + */ > > +void os_close_all_open_fd(int minfd) > > +{ > > + struct dirent *de; > > + int fd, dfd; > > + DIR *dir; > > + > > +#ifdef CONFIG_CLOSE_RANGE > > + int r = close_range(minfd, ~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 >= minfd && fd != dfd) { > > + close(fd); > > + } > > + } > > + closedir(dir); > > +} > > I think the way using sysconf(_SC_OPEN_MAX) is more portable, simpler and > cleaner than the one using /proc/self/fd.
A fallback that uses _SC_OPEN_MAX is good for portability, but it is should not be considered a replacement for iterating over /proc/self/fd, rather an additional fallback for non-Linux, or when /proc is not mounted. It is not uncommon for _SC_OPEN_MAX to be *exceedingly* high $ podman run -it quay.io/centos/centos:stream9 [root@4a440d62935c /]# ulimit -n 524288 Iterating over 1/2 a million FDs is a serious performance penalty that we don't want to have, so _SC_OPEN_MAX should always be the last resort. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
