On 8/28/24 14:39, Daniel P. Berrangé wrote: > On Wed, Aug 28, 2024 at 02:16:16PM +0200, Michal Privoznik wrote: >> The point of calling sysconf(_SC_OPEN_MAX) is to allocate big >> enough bitmap so that subsequent call to >> virCommandMassCloseGetFDsDir() can just set the bit instead of >> expanding memory (this code runs in a forked off child and thus >> using async-signal-unsafe functions like malloc() is a bit >> tricky). >> >> But on some systems the limit for opened FDs is virtually >> non-existent (typically macOS Ventura started reporting EINVAL). >> >> But with both glibc and musl using malloc() after fork() is safe. >> And with sufficiently new glib too, as it's using malloc() with >> newer releases instead of their own allocator. >> >> Therefore, pick a sufficiently large value (glibc falls back to >> 256, [1], so 1024 should be good enough) to fall back to and make >> the error non-fatal. >> >> 1: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/getdtsz.c;h=4c5a6208067d2f9eaaac6dba652702fb4af9b7e3;hb=HEAD >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> src/util/vircommand.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/src/util/vircommand.c b/src/util/vircommand.c >> index c0aab85c53..e48f361114 100644 >> --- a/src/util/vircommand.c >> +++ b/src/util/vircommand.c >> @@ -493,7 +493,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds, >> return -1; >> } >> >> - ignore_value(virBitmapSetBit(fds, fd)); >> + virBitmapSetBitExpand(fds, fd); >> } >> >> if (rc < 0) >> @@ -537,10 +537,8 @@ virCommandMassCloseFrom(virCommand *cmd, >> * Therefore we can safely allocate memory here (and transitively call >> * opendir/readdir) without a deadlock. */ >> >> t - if (openmax < 0) { >> - virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed")); >> - return -1; >> - } >> + if (openmax <= 0) >> + openmax = 1024; > > Darwin has a OPEN_MAX constant that is x10 bigger than this: > > https://github.com/apple/darwin-xnu/blob/main/bsd/sys/syslimits.h#L104 > > IMHO we should be using that instead Fair enough. virBitmapSetBitExpand() would cause realloc of the bitmap, but we can start with a generous value. Consider the following squashed in: diff --git i/src/util/vircommand.c w/src/util/vircommand.c index e48f361114..0f2f87c80c 100644 --- i/src/util/vircommand.c +++ w/src/util/vircommand.c @@ -537,8 +537,12 @@ virCommandMassCloseFrom(virCommand *cmd, * Therefore we can safely allocate memory here (and transitively call * opendir/readdir) without a deadlock. */ - if (openmax <= 0) - openmax = 1024; + if (openmax <= 0) { + /* Darwin defaults to 10240. Start with a generous value. + * virCommandMassCloseGetFDsDir() uses virBitmapSetBitExpand() anyways. + */ + openmax = 10240; + } fds = virBitmapNew(openmax); Michal
