Both virCommandMassCloseGetFDsLinux() and virCommandMassCloseGetFDsGeneric() take @cmd argument only to mark it as unused. Drop it from both. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/vircommand.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index a03fcc37ae..8bd5e5e771 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -478,8 +478,7 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups) * onto child process (well, the one we will exec soon since this * is called from the child). */ static int -virCommandMassCloseGetFDsLinux(virCommand *cmd G_GNUC_UNUSED, - virBitmap *fds) +virCommandMassCloseGetFDsLinux(virBitmap *fds) { g_autoptr(DIR) dp = NULL; struct dirent *entry; @@ -511,8 +510,7 @@ virCommandMassCloseGetFDsLinux(virCommand *cmd G_GNUC_UNUSED, # else /* !__linux__ */ static int -virCommandMassCloseGetFDsGeneric(virCommand *cmd G_GNUC_UNUSED, - virBitmap *fds) +virCommandMassCloseGetFDsGeneric(virBitmap *fds) { virBitmapSetAll(fds); return 0; @@ -547,10 +545,10 @@ virCommandMassCloseFrom(virCommand *cmd, fds = virBitmapNew(openmax); # ifdef __linux__ - if (virCommandMassCloseGetFDsLinux(cmd, fds) < 0) + if (virCommandMassCloseGetFDsLinux(fds) < 0) return -1; # else - if (virCommandMassCloseGetFDsGeneric(cmd, fds) < 0) + if (virCommandMassCloseGetFDsGeneric(fds) < 0) return -1; # endif -- 2.44.2

So far, virCommandMassCloseGetFDsLinux() opens "/proc/self/fd", iterates over it marking opened FDs in @fds bitmap. Well, we can do the same on other systems (with altered path), like MacOS or FreeBSD. Therefore, isolate dir iteration into a separate function that accepts dir path as an argument. Unfortunately, this function might be unused on some systems (e.g. mingw), therefore mark it as such. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/vircommand.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 8bd5e5e771..c0aab85c53 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -472,17 +472,12 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups) return 0; } -# ifdef __linux__ -/* On Linux, we can utilize procfs and read the table of opened - * FDs and selectively close only those FDs we don't want to pass - * onto child process (well, the one we will exec soon since this - * is called from the child). */ -static int -virCommandMassCloseGetFDsLinux(virBitmap *fds) +static int G_GNUC_UNUSED +virCommandMassCloseGetFDsDir(virBitmap *fds, + const char *dirName) { g_autoptr(DIR) dp = NULL; struct dirent *entry; - const char *dirName = "/proc/self/fd"; int rc; if (virDirOpen(&dp, dirName) < 0) @@ -507,15 +502,20 @@ virCommandMassCloseGetFDsLinux(virBitmap *fds) return 0; } -# else /* !__linux__ */ - static int -virCommandMassCloseGetFDsGeneric(virBitmap *fds) +virCommandMassCloseGetFDs(virBitmap *fds) { +# ifdef __linux__ + /* On Linux, we can utilize procfs and read the table of opened + * FDs and selectively close only those FDs we don't want to pass + * onto child process (well, the one we will exec soon since this + * is called from the child). */ + return virCommandMassCloseGetFDsDir(fds, "/proc/self/fd"); +# else virBitmapSetAll(fds); return 0; +# endif } -# endif /* !__linux__ */ static int virCommandMassCloseFrom(virCommand *cmd, @@ -544,13 +544,8 @@ virCommandMassCloseFrom(virCommand *cmd, fds = virBitmapNew(openmax); -# ifdef __linux__ - if (virCommandMassCloseGetFDsLinux(fds) < 0) + if (virCommandMassCloseGetFDs(fds) < 0) return -1; -# else - if (virCommandMassCloseGetFDsGeneric(fds) < 0) - return -1; -# endif lastfd = MAX(lastfd, childin); lastfd = MAX(lastfd, childout); -- 2.44.2

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/... Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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. */ - if (openmax < 0) { - virReportSystemError(errno, "%s", _("sysconf(_SC_OPEN_MAX) failed")); - return -1; - } + if (openmax <= 0) + openmax = 1024; fds = virBitmapNew(openmax); -- 2.44.2

On 8/28/24 14:39, Daniel P. Berrangé wrote: 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

On BSD-like systems "/dev/fd" serves the same purpose as "/proc/self/fd". And since procfs is usually not mounted, on such systems we can use "/dev/fd" instead. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/518 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index e48f361114..d48bf8122d 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -511,6 +511,8 @@ virCommandMassCloseGetFDs(virBitmap *fds) * onto child process (well, the one we will exec soon since this * is called from the child). */ return virCommandMassCloseGetFDsDir(fds, "/proc/self/fd"); +# elif defined(__APPLE__) || defined(__FreeBSD__) + return virCommandMassCloseGetFDsDir(fds, "/dev/fd"); # else virBitmapSetAll(fds); return 0; -- 2.44.2