| Message ID | 20240830111451.3799490-3-cleger@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | oslib: fix OSes support for qemu_close_all_open_fd() | expand |
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
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
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
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 > >
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 >> >> >
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 >>> >>> >> >
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 >>>> >>>> >>> >> >
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
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
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.
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);
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(+)