[PATCH 0/4] vircommand: Various mass close improvements
*** BLURB HERE *** Michal Prívozník (4): vircommand: Drop unused arguments from virCommandMassCloseGetFDs*() vircommand: Isolate FD dir parsing into a separate function vircommand: Make sysconf(_SC_OPEN_MAX) failure non-fatal vircommand: Parse /dev/fd on *BSD-like systems when looking for opened FDs src/util/vircommand.c | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) -- 2.41.0
Both virCommandMassCloseGetFDsLinux() and virCommandMassCloseGetFDsGeneric() take @cmd argument only to mark it as unused. Drop it from both. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 5f094c625a..60d419a695 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -485,8 +485,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; @@ -518,8 +517,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; @@ -554,10 +552,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.41.0
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@redhat.com> --- 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 60d419a695..822b9487f9 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -479,17 +479,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) @@ -514,15 +509,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, @@ -551,13 +551,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.41.0
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/get... Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 822b9487f9..8c06e19723 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -500,7 +500,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds, return -1; } - ignore_value(virBitmapSetBit(fds, fd)); + virBitmapSetBitExpand(fds, fd); } if (rc < 0) @@ -544,10 +544,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.41.0
On Tue, Aug 29, 2023 at 05:13:08PM +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.
That's not suitable for macOS. THey have a constant OPEN_MAX we should be using that is way bigger than 1024.
1: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/get... Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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 822b9487f9..8c06e19723 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -500,7 +500,7 @@ virCommandMassCloseGetFDsDir(virBitmap *fds, return -1; }
- ignore_value(virBitmapSetBit(fds, fd)); + virBitmapSetBitExpand(fds, fd); }
if (rc < 0) @@ -544,10 +544,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.41.0
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 :|
On 8/29/23 17:20, Daniel P. Berrangé wrote:
On Tue, Aug 29, 2023 at 05:13:08PM +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.
That's not suitable for macOS. THey have a constant OPEN_MAX we should be using that is way bigger than 1024.
Yeah, it's 2^63-1 per: https://github.com/eradman/entr/issues/63#issuecomment-776758771 If we really use that it's going to take ages to iterate over all FDs. OTOH, the next commit switches over to /dev/fd where we can learn about opened FDs, so it's not that bad. Except we would be allocating humongous virBitmap (1 EiB). what seems to be the right default then? Michal
On Wed, Aug 30, 2023 at 08:26:02AM +0200, Michal Prívozník wrote:
On 8/29/23 17:20, Daniel P. Berrangé wrote:
On Tue, Aug 29, 2023 at 05:13:08PM +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.
That's not suitable for macOS. THey have a constant OPEN_MAX we should be using that is way bigger than 1024.
Yeah, it's 2^63-1 per:
https://github.com/eradman/entr/issues/63#issuecomment-776758771
IIUC, that's referring to the result of sysconf(_SC_OPEN_MAX) which is returning -1. I was refering to a literal constant OPEN_MAX which is reported as being 10k based on the debug logs shown in commit message of https://gitlab.com/libvirt/libvirt/-/merge_requests/283 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 :|
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@redhat.com> --- src/util/vircommand.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 8c06e19723..2b3b125e5f 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -518,6 +518,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.41.0
participants (3)
-
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník