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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 <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.

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 :|