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

[Clément Léger]

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:
>> 20240716144006.6571-1-cleger@rivosinc.com/">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.

What's your advice on that ?

Thanks,

Clément