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

[Philippe Mathieu-Daudé]

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

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