Re: [PATCH 2/2] qemu/osdep: handle sysconf(_SC_OPEN_MAX) return value == -1

["Clément Léger" <cleger@rivosinc.com>]

On 02/09/2024 21:38, Philippe Mathieu-Daudé wrote:
> On 30/8/24 13:57, Clément Léger wrote:
>> On 30/08/2024 13:31, Michael Tokarev wrote:
>>> 30.08.2024 14:14, Clément Léger wrote:
>>>> On some systems (MacOS for instance), sysconf(_SC_OPEN_MAX) can return
>>>> -1. In that case we should fallback to using the OPEN_MAX define.
>>>> According to "man sysconf", the OPEN_MAX define should be present and
>>>> provided by either unistd.h and/or limits.h so include them for that
>>>> purpose. For other OSes, just assume a maximum of 1024 files
>>>> descriptors
>>>> as a fallback.
>>>>
>>>> Fixes: 4ec5ebea078e ("qemu/osdep: Move close_all_open_fds() to oslib-
>>>> posix")
>>>> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>
>>> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
>>>
>>>> @@ -928,6 +933,13 @@ static void qemu_close_all_open_fd_fallback(const
>>>> int *skip, unsigned int nskip,
>>>>    void qemu_close_all_open_fd(const int *skip, unsigned int nskip)
>>>>    {
>>>>        int open_max = sysconf(_SC_OPEN_MAX);
>>>> +    if (open_max == -1) {
>>>> +#ifdef CONFIG_DARWIN
>>>> +        open_max = OPEN_MAX;
> 
> Missing errno check.

man sysconf states that:

"On error, -1 is returned and errno is set to indicate the error (for
example, EINVAL, indicating that name is invalid)."

So it seems checking for -1 is enough no ? Or were you thinking about
something else ?

> 
>>>> +#else
>>>> +        open_max = 1024;
>>>> +#endif
>>>
>>> BTW, Can we PLEASE cap this to 1024 in all cases? :)
>>> (unrelated to this change but still).
>>
>> Hi Michael,
>>
>> Do you mean for all OSes or always using 1024 rather than using the
>> sysconf returned value ?
> 
> Alternatively add:
> 
>   long qemu_sysconf(int name, long unsupported_default);
> 
> which returns value, unsupported_default if not supported, or -1.

Acked, should this be a global function even if only used in the
qemu_close_all_open_fd() helper yet ?

Thanks,

Clément

> 
>>
>> In any case, the code now uses close_range() or /proc/self/fd and is
>> handling that efficiently.
>>
>> Thanks,
>>
>> Clément
>>
>>>
>>> /mjt
>>
>>
>