Re: [PATCH v4] osdep: add a qemu_close_all_open_fd() helper

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

On Tue, Jul 23, 2024 at 09:16:15AM +0200, Clément Léger wrote:
> 
> 
> On 23/07/2024 08:24, Philippe Mathieu-Daudé wrote:
> > Hi Clément,
> > 
> > On 17/7/24 14:45, Clément Léger wrote:
> >> 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.
> >>
> >> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >> ----
> > 
> > FYI git tools parse 3 '-', not 4.
> 
> Hi Philippe,
> 
> Thanks for the info, I was not aware of that ! I'll use 3 '-' from now on.
> 
> > 
> >> v4:
> >>   - Add a comment saying that qemu_close_all_fds() can take a NULL skip
> >>     array and nskip == 0
> >>   - Added an assert in qemu_close_all_fds() 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/
> > 
> > 
> >> +void qemu_close_all_open_fd(const int *skip, unsigned int nskip)
> >> +{
> >> +    int open_max = sysconf(_SC_OPEN_MAX);
> >> +    unsigned int cur_skip = 0;
> >> +    int i;
> >> +
> >> +    assert(skip != NULL || nskip == 0);
> >> +
> >> +    if (qemu_close_all_open_fd_close_range(skip, nskip)) {
> >> +        return;
> >> +    }
> >> +
> >> +    if (qemu_close_all_open_fd_proc(skip, nskip)) {
> >> +        return;
> >> +    }
> >> +
> >> +    /* Fallback */
> >> +    for (i = 0; i < open_max; i++) {
> >> +        if (cur_skip < nskip && i == skip[cur_skip]) {
> >> +            cur_skip++;
> >> +            continue;
> >> +        }
> >> +        close(i);
> >> +    }
> >> +}
> > 
> > Build failure on windows:
> > 
> > ../util/osdep.c: In function 'qemu_close_all_open_fd':
> > ../util/osdep.c:725:20: error: implicit declaration of function
> > 'sysconf'; did you mean 'swscanf'? [-Wimplicit-function-declaration]
> >   725 |     int open_max = sysconf(_SC_OPEN_MAX);
> >       |                    ^~~~~~~
> >       |                    swscanf
> > ../util/osdep.c:725:20: error: nested extern declaration of 'sysconf'
> > [-Werror=nested-externs]
> > ../util/osdep.c:725:28: error: '_SC_OPEN_MAX' undeclared (first use in
> > this function); did you mean 'FOPEN_MAX'?
> >   725 |     int open_max = sysconf(_SC_OPEN_MAX);
> >       |                            ^~~~~~~~~~~~
> >       |                            FOPEN_MAX
> > ../util/osdep.c:725:28: note: each undeclared identifier is reported
> > only once for each function it appears in
> > 
> 
> Should I move this chunk of code in oslib-posix.c and stub that function
> for win32 ? It seems like this code was not built for windows before I
> added it in osdep, which means it is not needed for win32.

Yes, I think that'll be OK. The fork/exec concept isn't present on Windows
so we also don't need to close all FDs.


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