[PATCH 0/2] oslib: fix OSes support for qemu_close_all_open_fd()

[PATCH 0/2] oslib: fix OSes support for qemu_close_all_open_fd()

[Clément Léger]

Some OSes (MacOS, FreeBSD) were not correctly handled by the original
close_fd() code. Handle them by fixing the path for self process open
file descriptors location as well as sysconf(_SC_OPEN_MAX) potentially
returning -1 on MacOS.

---

Clément Léger (2):
  qemu/osdep: fix current process fds path for other OSes
  qemu/osdep: handle sysconf(_SC_OPEN_MAX) return value == -1

 util/oslib-posix.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

-- 
2.45.2

[PATCH 1/2] qemu/osdep: fix current process fds path for other OSes

[Clément Léger]

While Linux and Solaris uses /proc/self/fd, other OSes (MacOS,
FreeBSD) uses /dev/fd. In order to support theses OSes, use /dev/fd 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>
---
 util/oslib-posix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 11b35e48fb..901dcccd73 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -814,8 +814,13 @@ static bool qemu_close_all_open_fd_proc(const int *skip, unsigned int nskip)
     int fd, dfd;
     DIR *dir;
     unsigned int skip_start = 0, skip_end = nskip;
+#if defined(CONFIG_LINUX) || defined(CONFIG_SOLARIS)
+    const char *self_fd = "/proc/self/fd";
+#else
+    const char *self_fd = "/dev/fd";
+#endif
 
-    dir = opendir("/proc/self/fd");
+    dir = opendir(self_fd);
     if (!dir) {
         /* If /proc is not mounted, there is nothing that can be done. */
         return false;
-- 
2.45.2

Re: [PATCH 1/2] qemu/osdep: fix current process fds path for other OSes

[Daniel P. Berrangé]

On Fri, Aug 30, 2024 at 01:14:49PM +0200, Clément Léger wrote:
> While Linux and Solaris uses /proc/self/fd, other OSes (MacOS,
> FreeBSD) uses /dev/fd. In order to support theses OSes, use /dev/fd 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>
> ---
>  util/oslib-posix.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

Re: [PATCH 1/2] qemu/osdep: fix current process fds path for other OSes

[Michael Tokarev]

30.08.2024 14:14, Clément Léger:
> While Linux and Solaris uses /proc/self/fd, other OSes (MacOS,
> FreeBSD) uses /dev/fd. In order to support theses OSes, use /dev/fd 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>

/mjt

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

[Clément Léger]

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>
---
 util/oslib-posix.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 901dcccd73..abf3aa597d 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -44,6 +44,11 @@
 #include "qemu/thread-context.h"
 #include "qemu/main-loop.h"
 
+#ifdef CONFIG_DARWIN
+#include <limits.h>
+#include <unistd.h>
+#endif
+
 #ifdef CONFIG_LINUX
 #include <sys/syscall.h>
 #endif
@@ -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;
+#else
+        open_max = 1024;
+#endif
+    }
 
     assert(skip != NULL || nskip == 0);
 
-- 
2.45.2

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

[Daniel P. Berrangé]

On Fri, Aug 30, 2024 at 01:14:50PM +0200, 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>
> ---
>  util/oslib-posix.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

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

[Michael Tokarev]

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;
> +#else
> +        open_max = 1024;
> +#endif

BTW, Can we PLEASE cap this to 1024 in all cases? :)
(unrelated to this change but still).

/mjt

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

[Clément Léger]

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;
>> +#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 ?

In any case, the code now uses close_range() or /proc/self/fd and is
handling that efficiently.

Thanks,

Clément

> 
> /mjt

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

[Philippe Mathieu-Daudé]

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.

>>> +#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.

> 
> In any case, the code now uses close_range() or /proc/self/fd and is
> handling that efficiently.
> 
> Thanks,
> 
> Clément
> 
>>
>> /mjt
> 
> 

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

[Clément Léger]

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 ?

> 
>>>> +#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 ?

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

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

[Philippe Mathieu-Daudé]

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

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

[Clément Léger]

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.

> 
>  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
>>
>>>
>>>>
>>>> In any case, the code now uses close_range() or /proc/self/fd and is
>>>> handling that efficiently.
>>>>
>>>> Thanks,
>>>>
>>>> Clément
>>>>
>>>>>
>>>>> /mjt
>>>>
>>>>
>>>
>>
> 

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

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

[Daniel P. Berrangé]

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

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

[Philippe Mathieu-Daudé]

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.