9:09 a.m.
This is basically a v2 of:
https://listman.redhat.com/archives/libvir-list/2023-June/240320.html
But after discussion with Dan and most importantly, once he found the
root cause we can do proper fixes instead of papering over the issue.
Michal Prívozník (3):
vircommand: Use closefrom() more often
vircommand: Utilize close_range()
virGlobalInit: Make glib init its own global state
meson.build | 2 +
src/libvirt.c | 8 ++
src/util/vircommand.c | 239 +++++++++++++++++++++++++++++++-----------
3 files changed, 190 insertions(+), 59 deletions(-)
--
2.39.3
9:09 a.m.
New subject: [PATCH 1/3] vircommand: Use closefrom() more often
As of commit v5.9-rc1~160^2~3 the Linux kernel has close_range()
syscall, which closes not just one FD but whole range. Then, in
its commit glibc-2.34~115 glibc introduced closefrom() which is
just a wrapper over close_range(), but it allows us to use
FreeBSD-only implementation on Linux too, as both OS-es now have
the same function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
meson.build | 1 +
src/util/vircommand.c | 124 ++++++++++++++++++++++--------------------
2 files changed, 66 insertions(+), 59 deletions(-)
diff --git a/meson.build b/meson.build
index aa391e7178..a4b52b6156 100644
--- a/meson.build
+++ b/meson.build
@@ -573,6 +573,7 @@ libvirt_export_dynamic = cc.first_supported_link_argument([
# check availability of various common functions (non-fatal if missing)
functions = [
+ 'closefrom',
'elf_aux_info',
'explicit_bzero',
'fallocate',
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 49abb53c28..b8b8d48f92 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -479,7 +479,68 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
return 0;
}
-# ifdef __linux__
+# ifdef WITH_CLOSEFROM
+# define USE_CLOSEFROM
+# else
+# define USE_GENERIC
+# endif
+
+
+# ifdef USE_CLOSEFROM
+static int
+virCommandMassClose(virCommand *cmd,
+ int childin,
+ int childout,
+ int childerr)
+{
+ int lastfd = -1;
+ int fd = -1;
+ size_t i;
+
+ /*
+ * Two phases of closing.
+ *
+ * The first (inefficient) phase iterates over FDs,
+ * preserving certain FDs we need to pass down, and
+ * closing others. The number of iterations is bounded
+ * to the number of the biggest FD we need to preserve.
+ *
+ * The second (speedy) phase uses closefrom() to cull
+ * all remaining FDs in the process.
+ *
+ * Usually the first phase will be fairly quick only
+ * processing a handful of low FD numbers, and thus using
+ * closefrom() is a massive win for high ulimit() NFILES
+ * values.
+ */
+ lastfd = MAX(lastfd, childin);
+ lastfd = MAX(lastfd, childout);
+ lastfd = MAX(lastfd, childerr);
+
+ for (i = 0; i < cmd->npassfd; i++)
+ lastfd = MAX(lastfd, cmd->passfd[i].fd);
+
+ for (fd = 0; fd <= lastfd; fd++) {
+ if (fd == childin || fd == childout || fd == childerr)
+ continue;
+ if (!virCommandFDIsSet(cmd, fd)) {
+ int tmpfd = fd;
+ VIR_MASS_CLOSE(tmpfd);
+ } else if (virSetInherit(fd, true) < 0) {
+ virReportSystemError(errno, _("failed to preserve fd %1$d"), fd);
+ return -1;
+ }
+ }
+
+ closefrom(lastfd + 1);
+
+ return 0;
+}
+# endif /* ! WITH_CLOSEFROM */
+
+
+# ifdef USE_GENERIC
+# 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
@@ -515,7 +576,7 @@ virCommandMassCloseGetFDsLinux(virCommand *cmd G_GNUC_UNUSED,
return 0;
}
-# else /* !__linux__ */
+# else /* !__linux__ */
static int
virCommandMassCloseGetFDsGeneric(virCommand *cmd G_GNUC_UNUSED,
@@ -524,61 +585,7 @@ virCommandMassCloseGetFDsGeneric(virCommand *cmd G_GNUC_UNUSED,
virBitmapSetAll(fds);
return 0;
}
-# endif /* !__linux__ */
-
-# ifdef __FreeBSD__
-
-static int
-virCommandMassClose(virCommand *cmd,
- int childin,
- int childout,
- int childerr)
-{
- int lastfd = -1;
- int fd = -1;
- size_t i;
-
- /*
- * Two phases of closing.
- *
- * The first (inefficient) phase iterates over FDs,
- * preserving certain FDs we need to pass down, and
- * closing others. The number of iterations is bounded
- * to the number of the biggest FD we need to preserve.
- *
- * The second (speedy) phase uses closefrom() to cull
- * all remaining FDs in the process.
- *
- * Usually the first phase will be fairly quick only
- * processing a handful of low FD numbers, and thus using
- * closefrom() is a massive win for high ulimit() NFILES
- * values.
- */
- lastfd = MAX(lastfd, childin);
- lastfd = MAX(lastfd, childout);
- lastfd = MAX(lastfd, childerr);
-
- for (i = 0; i < cmd->npassfd; i++)
- lastfd = MAX(lastfd, cmd->passfd[i].fd);
-
- for (fd = 0; fd <= lastfd; fd++) {
- if (fd == childin || fd == childout || fd == childerr)
- continue;
- if (!virCommandFDIsSet(cmd, fd)) {
- int tmpfd = fd;
- VIR_MASS_CLOSE(tmpfd);
- } else if (virSetInherit(fd, true) < 0) {
- virReportSystemError(errno, _("failed to preserve fd %1$d"), fd);
- return -1;
- }
- }
-
- closefrom(lastfd + 1);
-
- return 0;
-}
-
-# else /* ! __FreeBSD__ */
+# endif /* !__linux__ */
static int
virCommandMassClose(virCommand *cmd,
@@ -628,8 +635,7 @@ virCommandMassClose(virCommand *cmd,
return 0;
}
-
-# endif /* ! __FreeBSD__ */
+# endif /* ! USE_GENERIC */
/*
--
2.39.3
10:34 a.m.
New subject: [PATCH 1/3] vircommand: Use closefrom() more often
On Wed, Jun 21, 2023 at 04:09:09PM +0200, Michal Privoznik wrote:
As of commit v5.9-rc1~160^2~3 the Linux kernel has close_range()
syscall, which closes not just one FD but whole range. Then, in
its commit glibc-2.34~115 glibc introduced closefrom() which is
just a wrapper over close_range(), but it allows us to use
FreeBSD-only implementation on Linux too, as both OS-es now have
the same function.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
meson.build | 1 +
src/util/vircommand.c | 124 ++++++++++++++++++++++--------------------
2 files changed, 66 insertions(+), 59 deletions(-)
diff --git a/meson.build b/meson.build
index aa391e7178..a4b52b6156 100644
--- a/meson.build
+++ b/meson.build
@@ -573,6 +573,7 @@ libvirt_export_dynamic = cc.first_supported_link_argument([
# check availability of various common functions (non-fatal if missing)
functions = [
+ 'closefrom',
'elf_aux_info',
'explicit_bzero',
'fallocate',
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 49abb53c28..b8b8d48f92 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -479,7 +479,68 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
return 0;
}
-# ifdef __linux__
+# ifdef WITH_CLOSEFROM
+# define USE_CLOSEFROM
+# else
+# define USE_GENERIC
+# endif
+
+
+# ifdef USE_CLOSEFROM
+static int
+virCommandMassClose(virCommand *cmd,
+ int childin,
+ int childout,
+ int childerr)
+{
+ int lastfd = -1;
+ int fd = -1;
+ size_t i;
+
+ /*
+ * Two phases of closing.
+ *
+ * The first (inefficient) phase iterates over FDs,
+ * preserving certain FDs we need to pass down, and
+ * closing others. The number of iterations is bounded
+ * to the number of the biggest FD we need to preserve.
+ *
+ * The second (speedy) phase uses closefrom() to cull
+ * all remaining FDs in the process.
+ *
+ * Usually the first phase will be fairly quick only
+ * processing a handful of low FD numbers, and thus using
+ * closefrom() is a massive win for high ulimit() NFILES
+ * values.
+ */
+ lastfd = MAX(lastfd, childin);
+ lastfd = MAX(lastfd, childout);
+ lastfd = MAX(lastfd, childerr);
+
+ for (i = 0; i < cmd->npassfd; i++)
+ lastfd = MAX(lastfd, cmd->passfd[i].fd);
+
+ for (fd = 0; fd <= lastfd; fd++) {
+ if (fd == childin || fd == childout || fd == childerr)
+ continue;
+ if (!virCommandFDIsSet(cmd, fd)) {
+ int tmpfd = fd;
+ VIR_MASS_CLOSE(tmpfd);
+ } else if (virSetInherit(fd, true) < 0) {
+ virReportSystemError(errno, _("failed to preserve fd %1$d"), fd);
+ return -1;
+ }
+ }
+
+ closefrom(lastfd + 1);
GLibC might have closefrom() but be in a container running on an
older kernel that lacks close_range syscall.
We could ignore return value on FreeBSD, but now on Linux we must
check the return value and fallback to the old codepath(s).
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 :|
9:09 a.m.
New subject: [PATCH 2/3] vircommand: Utilize close_range()
As of commit v5.9-rc1~160^2~3 the Linux kernel has close_range()
syscall, which closes not just one FD but whole range. In glibc
this is exposed by automatically generated wrapper of the same
name. In musl, this is not exposed, yet, but we can call the
syscall() directly. In either case, we have to deal with a
situation, when the kernel we're running under does not have the
syscall as glibc deliberately does not implement fallback.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
meson.build | 1 +
src/util/vircommand.c | 117 +++++++++++++++++++++++++++++++++++++++++-
2 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index a4b52b6156..ecfc1b6bdf 100644
--- a/meson.build
+++ b/meson.build
@@ -573,6 +573,7 @@ libvirt_export_dynamic = cc.first_supported_link_argument([
# check availability of various common functions (non-fatal if missing)
functions = [
+ 'close_range',
'closefrom',
'elf_aux_info',
'explicit_bzero',
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index b8b8d48f92..e826f5f348 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -479,13 +479,128 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
return 0;
}
-# ifdef WITH_CLOSEFROM
+# if defined(WITH_CLOSE_RANGE) || \
+ (defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range))
+# define USE_CLOSE_RANGE
+# elif defined(WITH_CLOSEFROM)
# define USE_CLOSEFROM
# else
# define USE_GENERIC
# endif
+# ifdef USE_CLOSE_RANGE
+static int
+virCloseRange(unsigned int first,
+ unsigned int last,
+ unsigned int flags)
+{
+
+ static virTristateBool has_close_range = VIR_TRISTATE_BOOL_ABSENT;
+ int fd;
+ int ret = -1;
+
+ VIR_DEBUG("first=%u, last=%u, flags=0x%x, has_close_range=%d",
+ first, last, flags, has_close_range);
+
+ if (has_close_range != VIR_TRISTATE_BOOL_NO) {
+# if WITH_CLOSE_RANGE
+ ret = close_range(first, last, flags);
+# else
+ ret = syscall(SYS_close_range, first, last, flags);
+# endif
+ }
+
+ if (ret == 0) {
+ if (has_close_range == VIR_TRISTATE_BOOL_ABSENT)
+ has_close_range = VIR_TRISTATE_BOOL_YES;
+ return 0;
+ }
+
+ if (errno == ENOSYS) {
+ if (has_close_range == VIR_TRISTATE_BOOL_ABSENT) {
+ VIR_DEBUG("Kernel does not support close_range, falling back to naive
implementation");
+ has_close_range = VIR_TRISTATE_BOOL_NO;
+ }
+ } else {
+ return ret;
+ }
+
+ /* glibc does not implement fallback, we have to implement it ourselves. */
+ if (flags != 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ for (fd = first; fd <= last; fd++) {
+ int tmpfd = fd;
+ VIR_MASS_CLOSE(tmpfd);
+ }
+
+ return 0;
+}
+
+
+static int
+virCommandMassClose(virCommand *cmd,
+ int childin,
+ int childout,
+ int childerr)
+{
+ g_autoptr(virBitmap) fds = virBitmapNew(3);
+ ssize_t first;
+ ssize_t last;
+ int openmax = sysconf(_SC_OPEN_MAX);
+ size_t i;
+
+ virBitmapSetBitExpand(fds, childin);
+ virBitmapSetBitExpand(fds, childout);
+ virBitmapSetBitExpand(fds, childerr);
+
+ for (i = 0; i < cmd->npassfd; i++) {
+ int fd = cmd->passfd[i].fd;
+
+ virBitmapSetBitExpand(fds, fd);
+
+ if (virSetInherit(fd, true) < 0) {
+ virReportSystemError(errno, _("failed to preserve fd %1$d"), fd);
+ return -1;
+ }
+ }
+
+ first = 2;
+ while ((last = virBitmapNextSetBit(fds, first)) >= 0) {
+ if (first + 1 == last) {
+ first = last;
+ continue;
+ }
+
+ /* Preserve @first and @last and close everything in between. */
+ if (virCloseRange(first + 1, last - 1, 0) < 0) {
+ virReportSystemError(errno,
+ _("Unable to mass close FDs (first=%1$zd,
last=%2$zd)"),
+ first + 1, last - 1);
+ return -1;
+ }
+
+ first = last;
+ }
+
+ if (openmax < 0)
+ openmax = INT_MAX;
+
+ if (virCloseRange(first + 1, openmax, 0) < 0) {
+ virReportSystemError(errno,
+ _("Unable to mass close FDs (first=%1$zd,
last=%2$d"),
+ first + 1, openmax);
+ return -1;
+ }
+
+ return 0;
+}
+# endif /* USE_CLOSE_RANGE */
+
+
# ifdef USE_CLOSEFROM
static int
virCommandMassClose(virCommand *cmd,
--
2.39.3
10:40 a.m.
New subject: [PATCH 2/3] vircommand: Utilize close_range()
On Wed, Jun 21, 2023 at 04:09:10PM +0200, Michal Privoznik wrote:
As of commit v5.9-rc1~160^2~3 the Linux kernel has close_range()
syscall, which closes not just one FD but whole range. In glibc
this is exposed by automatically generated wrapper of the same
name. In musl, this is not exposed, yet, but we can call the
syscall() directly. In either case, we have to deal with a
situation, when the kernel we're running under does not have the
syscall as glibc deliberately does not implement fallback.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
meson.build | 1 +
src/util/vircommand.c | 117 +++++++++++++++++++++++++++++++++++++++++-
2 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index a4b52b6156..ecfc1b6bdf 100644
--- a/meson.build
+++ b/meson.build
@@ -573,6 +573,7 @@ libvirt_export_dynamic = cc.first_supported_link_argument([
# check availability of various common functions (non-fatal if missing)
functions = [
+ 'close_range',
'closefrom',
'elf_aux_info',
'explicit_bzero',
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index b8b8d48f92..e826f5f348 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -479,13 +479,128 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
return 0;
}
-# ifdef WITH_CLOSEFROM
+# if defined(WITH_CLOSE_RANGE) || \
+ (defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range))
+# define USE_CLOSE_RANGE
+# elif defined(WITH_CLOSEFROM)
# define USE_CLOSEFROM
# else
# define USE_GENERIC
# endif
Although you've optimized to take advantage of using close_range
to deal with gaps, in the interests of code brevity, I wonder if
we shouldn't simplify this patch down to merely:
#ifndef WITH_CLOSEFROM
# ifdef WITH_CLOSE_RANGE
# define closefrom(n) closerange(n, ~0U, 0)
# define WITH_CLOSEFROM
# else if defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range)
# define closefrom(n) syscall(SYS_close_range, n, ~0U, 0)
# define WITH_CLOSEFROM
# endif
#endif
thus just using the existing closefrom() impl (assuming you adjust the
previous patch to check return status for ENOSYS and do legacy fallback.
Not convinced it is worth worrying about the FD gaps ?
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 :|
10:38 a.m.
New subject: [PATCH 2/3] vircommand: Utilize close_range()
On Wed, Jun 21, 2023 at 04:40:10PM +0100, Daniel P. Berrangé wrote:
On Wed, Jun 21, 2023 at 04:09:10PM +0200, Michal Privoznik wrote:
> As of commit v5.9-rc1~160^2~3 the Linux kernel has close_range()
> syscall, which closes not just one FD but whole range. In glibc
> this is exposed by automatically generated wrapper of the same
> name. In musl, this is not exposed, yet, but we can call the
> syscall() directly. In either case, we have to deal with a
> situation, when the kernel we're running under does not have the
> syscall as glibc deliberately does not implement fallback.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> meson.build | 1 +
> src/util/vircommand.c | 117 +++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 117 insertions(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index a4b52b6156..ecfc1b6bdf 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -573,6 +573,7 @@ libvirt_export_dynamic = cc.first_supported_link_argument([
> # check availability of various common functions (non-fatal if missing)
>
> functions = [
> + 'close_range',
> 'closefrom',
> 'elf_aux_info',
> 'explicit_bzero',
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index b8b8d48f92..e826f5f348 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -479,13 +479,128 @@ virExecCommon(virCommand *cmd, gid_t *groups, int ngroups)
> return 0;
> }
>
> -# ifdef WITH_CLOSEFROM
> +# if defined(WITH_CLOSE_RANGE) || \
> + (defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range))
> +# define USE_CLOSE_RANGE
> +# elif defined(WITH_CLOSEFROM)
> # define USE_CLOSEFROM
> # else
> # define USE_GENERIC
> # endif
Although you've optimized to take advantage of using close_range
to deal with gaps, in the interests of code brevity, I wonder if
we shouldn't simplify this patch down to merely:
#ifndef WITH_CLOSEFROM
# ifdef WITH_CLOSE_RANGE
# define closefrom(n) closerange(n, ~0U, 0)
# define WITH_CLOSEFROM
# else if defined(WITH_SYS_SYSCALL_H) && defined(SYS_close_range)
# define closefrom(n) syscall(SYS_close_range, n, ~0U, 0)
# define WITH_CLOSEFROM
# endif
#endif
thus just using the existing closefrom() impl (assuming you adjust the
previous patch to check return status for ENOSYS and do legacy fallback.
Not convinced it is worth worrying about the FD gaps ?
Changing my mind on this. I learnt that FreeBSD added support for
close_range() to match Linux, since it was considered more flexible
than their previous closefrom().
This got into FreeBSD in Apr 2020, and IIUC they backported it to
12.2, though worst case it is in 12.3
Given our supported platforms matrix I think we can assume any FreeBSD
we target has close_range.
IOW, we don't need to support closefrom() on FreeBSD any more.
Thus I'd suggest we exclusively use close_range in livirt for both
Linux and FreeBSD in the fast path, henceforth.
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 :|
9:09 a.m.
New subject: [PATCH 3/3] virGlobalInit: Make glib init its own global state
This should not be needed, but here's what's happening:
virStrToLong_*() family of functions was switched from strtol*()
to g_ascii_strtol*() in order to handle corner cases on Windows
(most notably parsing hex numbers with base=0) - see
v9.4.0-61-g2ed41d7cd9. But what we did not realize back then, is
the fact that g_ascii_strtol*() family has their own global lock
rendering virStrToLong_*() function unsafe between fork() +
exec(). Worse, if one of the threads has to wait for the lock (or
on its corresponding condition), then errno is mangled and
g_ascii_strtol*() signals an error, even though there's no error.
Read more here:
https://gitlab.gnome.org/GNOME/glib/-/issues/3034
Nevertheless, if we make glib init the g_ascii_strtol*() global
state (by calling one function from g_ascii_strtol*() family),
then there shouldn't be any congestion on the lock and thus no
errno mangling.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c
index 2e470adf98..69d5b13bff 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -214,6 +214,14 @@ virGlobalInit(void)
if (virErrorInitialize() < 0)
goto error;
+ /* Make glib initialize its own global state. See more:
+ *
+ * https://gitlab.gnome.org/GNOME/glib/-/issues/3034
+ *
+ * TODO: Remove ASAP.
+ */
+ g_ascii_strtoull("0", NULL, 0);
+
virFileActivateDirOverrideForLib();
if (getuid() != geteuid() ||
--
2.39.3
10:30 a.m.
New subject: [PATCH 3/3] virGlobalInit: Make glib init its own global state
On Wed, Jun 21, 2023 at 04:09:11PM +0200, Michal Privoznik wrote:
This should not be needed, but here's what's happening:
virStrToLong_*() family of functions was switched from strtol*()
to g_ascii_strtol*() in order to handle corner cases on Windows
(most notably parsing hex numbers with base=0) - see
v9.4.0-61-g2ed41d7cd9. But what we did not realize back then, is
the fact that g_ascii_strtol*() family has their own global lock
rendering virStrToLong_*() function unsafe between fork() +
exec(). Worse, if one of the threads has to wait for the lock (or
on its corresponding condition), then errno is mangled and
g_ascii_strtol*() signals an error, even though there's no error.
Read more here:
https://gitlab.gnome.org/GNOME/glib/-/issues/3034
Nevertheless, if we make glib init the g_ascii_strtol*() global
state (by calling one function from g_ascii_strtol*() family),
then there shouldn't be any congestion on the lock and thus no
errno mangling.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt.c | 8 ++++++++
1 file changed, 8 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
4:10 a.m.
New subject: [PATCH 3/3] virGlobalInit: Make glib init its own global state
On Wed, Jun 21, 2023 at 5:30 PM Daniel P. Berrangé <berrange(a)redhat.com> wrote:
On Wed, Jun 21, 2023 at 04:09:11PM +0200, Michal Privoznik wrote:
> This should not be needed, but here's what's happening:
> virStrToLong_*() family of functions was switched from strtol*()
> to g_ascii_strtol*() in order to handle corner cases on Windows
> (most notably parsing hex numbers with base=0) - see
> v9.4.0-61-g2ed41d7cd9. But what we did not realize back then, is
> the fact that g_ascii_strtol*() family has their own global lock
> rendering virStrToLong_*() function unsafe between fork() +
> exec(). Worse, if one of the threads has to wait for the lock (or
> on its corresponding condition), then errno is mangled and
> g_ascii_strtol*() signals an error, even though there's no error.
>
> Read more here:
>
> https://gitlab.gnome.org/GNOME/glib/-/issues/3034
>
> Nevertheless, if we make glib init the g_ascii_strtol*() global
> state (by calling one function from g_ascii_strtol*() family),
> then there shouldn't be any congestion on the lock and thus no
> errno mangling.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/libvirt.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Thanks, let me push this one, as it fixes both problems (daedlock and
string parse problem) and work on the other two next week as I'm going
AFK for the rest of the week. Jirka plans to freeze on Tuesday next
week but after this patch, close_range()/closefrom() is not that
critical.
Michal
4:16 a.m.
New subject: [PATCH 3/3] virGlobalInit: Make glib init its own global state
On Thu, Jun 22, 2023 at 11:10:25AM +0200, Michal Privoznik wrote:
On Wed, Jun 21, 2023 at 5:30 PM Daniel P. Berrangé
<berrange(a)redhat.com> wrote:
>
> On Wed, Jun 21, 2023 at 04:09:11PM +0200, Michal Privoznik wrote:
> > This should not be needed, but here's what's happening:
> > virStrToLong_*() family of functions was switched from strtol*()
> > to g_ascii_strtol*() in order to handle corner cases on Windows
> > (most notably parsing hex numbers with base=0) - see
> > v9.4.0-61-g2ed41d7cd9. But what we did not realize back then, is
> > the fact that g_ascii_strtol*() family has their own global lock
> > rendering virStrToLong_*() function unsafe between fork() +
> > exec(). Worse, if one of the threads has to wait for the lock (or
> > on its corresponding condition), then errno is mangled and
> > g_ascii_strtol*() signals an error, even though there's no error.
> >
> > Read more here:
> >
> > https://gitlab.gnome.org/GNOME/glib/-/issues/3034
> >
> > Nevertheless, if we make glib init the g_ascii_strtol*() global
> > state (by calling one function from g_ascii_strtol*() family),
> > then there shouldn't be any congestion on the lock and thus no
> > errno mangling.
> >
> > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> > ---
> > src/libvirt.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
>
> Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Thanks, let me push this one, as it fixes both problems (daedlock and
string parse problem) and work on the other two next week as I'm going
AFK for the rest of the week. Jirka plans to freeze on Tuesday next
week but after this patch, close_range()/closefrom() is not that
critical.
Sure, sounds fine to me.
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 :|
514
days inactive
521
days old
9 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Michal Privoznik