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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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é <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 ?

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.

  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().

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