On 03/09/2024 15:34, Philippe Mathieu-Daudé wrote:
> On 3/9/24 09:53, Clément Léger wrote:
>> 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 ?
>
> Mine (macOS 14.6) is:
>
> RETURN VALUES
> If the call to sysconf() is not successful, -1 is returned and
> errno is set appropriately. Otherwise, if the variable is
> associated with functionality that is not supported, -1 is
> returned and errno is not modified. Otherwise, the current
> variable value is returned.
Which seems to imply the same than mine right ? -1 is always returned in
case of error and errno might or not be set. So checking for -1 seems
enough to check an error return.
>
> STANDARDS
> Except for the fact that values returned by sysconf() may change
> over the lifetime of the calling process, this function conforms
> to IEEE Std 1003.1-1988 (“POSIX.1”).
>
>>
>>>
>>>>>> +#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 ?
>
> I'm seeing a few more:
>
> $ git grep -w sysconf | wc -l
> 35
>
> From this list a dozen could use qemu_sysconf().
Acked.
>
>>
>> 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
>>>>
>>>>
>>>
>>
>
