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

["Daniel P. Berrangé" <berrange@redhat.com>]

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

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|