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

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

On 3/9/24 15:37, Clément Léger wrote:
> 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é <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.
> 
> 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.

Yes but we can check for the unsupported case. Something like:

     long qemu_sysconf(int name, long unsupported_default)
     {
         int current_errno = errno;
         long retval;

         retval = sysconf(name);
         if (retval == -1) {
             if (errno == current_errno) {
                 return unsupported_default;
             }
             perror("sysconf");
             return -1;
         }
         return retval;
     }

(untested)

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