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

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

On 3/9/24 17:21, Daniel P. Berrangé wrote:
> On Tue, Sep 03, 2024 at 05:02:44PM +0200, Philippe Mathieu-Daudé wrote:
>> 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;
>>      }
> 
> That looks uncessarily complicated, and IMHO makes it less
> portable. eg consider macOS behaviour:
> 
>   "if the variable is associated with functionality that is
>    not supported, -1 is returned and errno is not modified"
> 
> vs Linux documented behaviour:
> 
>    "If name corresponds to a maximum or minimum limit, and
>     that limit is indeterminate, -1 is returned and errno
>     is  not  changed."
> 
> IMHO we should do what Clément already suggested and set a
> default anytime retval == -1, and ignore errno. There is
> no user benefit from turning errnos into a fatal error
> via perror()

Fine by me.