Re: [PATCH trivial 1/2] close_all_open_fd(): move to oslib-posix.c

[Laurent Vivier]

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 <mjt@tls.msk.ru>
> ---
>   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.

Thanks,
Laurent
> +
>   int qemu_daemon(int nochdir, int noclose)
>   {
>       return daemon(nochdir, noclose);