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é <berra...@redhat.com>
>>>> Signed-off-by: Clément Léger <cle...@rivosinc.com>
>>>
>>> Reviewed-by: Michael Tokarev <m...@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
>>
>>
>
