This new helper can be used to spawn a child process and run
passed callback from it. This will come handy esp. if the
callback is not thread safe.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virprocess.c | 81 ++++++++++++++++++++++++++++++++++++++++
src/util/virprocess.h | 16 ++++++++
3 files changed, 98 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 335210c31d..e2dc85310a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2625,6 +2625,7 @@ virProcessKill;
virProcessKillPainfully;
virProcessKillPainfullyDelay;
virProcessNamespaceAvailable;
+virProcessRunInFork;
virProcessRunInMountNamespace;
virProcessSchedPolicyTypeFromString;
virProcessSchedPolicyTypeToString;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 3883c708fc..51b9ccb1bb 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -1165,6 +1165,87 @@ virProcessRunInMountNamespace(pid_t pid,
}
+static int
+virProcessForkHelper(int errfd,
+ pid_t pid,
+ virProcessForkCallback cb,
+ void *opaque)
+{
+ if (cb(pid, opaque) < 0) {
+ virErrorPtr err = virGetLastError();
+ if (err) {
+ size_t len = strlen(err->message) + 1;
+ ignore_value(safewrite(errfd, err->message, len));
+ }
+
+ return -1;
+ }
+
+ return 0;
+}
+
+
+/**
+ * virProcessRunInFork:
+ * @cb: callback to run
+ * @opaque: opaque data to @cb
+ *
+ * Do the fork and run @cb in the child. This can be used when
+ * @cb does something thread unsafe, for instance. However, due
+ * to possible signal propagation @cb should only call async
+ * signal safe functions. On return, the returned value is either
+ * -1 with error message reported if something went bad in the
+ * parent, if child has died from a signal or if the child
+ * returned EXIT_CANCELED. Otherwise the returned value is the
+ * exit status of the child.
+ */
+int
+virProcessRunInFork(virProcessForkCallback cb,
+ void *opaque)
+{
+ int ret = -1;
+ pid_t child = -1;
+ pid_t parent = getpid();
+ int errfd[2] = { -1, -1 };
+
+ if (pipe2(errfd, O_CLOEXEC) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Cannot create pipe for child"));
+ return -1;
+ }
+
+ if ((child = virFork()) < 0)
+ goto cleanup;
+
+ if (child == 0) {
+ VIR_FORCE_CLOSE(errfd[0]);
+ ret = virProcessForkHelper(errfd[1], parent, cb, opaque);
+ VIR_FORCE_CLOSE(errfd[1]);
+ _exit(ret < 0 ? EXIT_CANCELED : ret);
+ } else {
+ int status;
+ VIR_AUTOFREE(char *) buf = NULL;
+
+ VIR_FORCE_CLOSE(errfd[1]);
+ ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
+ ret = virProcessWait(child, &status, false);
+ if (!ret) {
+ ret = status == EXIT_CANCELED ? -1 : status;
+ if (ret) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("child reported: %s"),
+ NULLSTR(buf));
+ }
+ }
+ }
+
+ cleanup:
+ VIR_FORCE_CLOSE(errfd[0]);
+ VIR_FORCE_CLOSE(errfd[1]);
+ return ret;
+}
+
+
#if defined(HAVE_SYS_MOUNT_H) && defined(HAVE_UNSHARE)
int
virProcessSetupPrivateMountNS(void)
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index 5faa0892fe..1bae8c9212 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -93,6 +93,22 @@ int virProcessRunInMountNamespace(pid_t pid,
virProcessNamespaceCallback cb,
void *opaque);
+/**
+ * virProcessForkCallback:
+ * @pid: parent's pid
+ * @opaque: opaque data
+ *
+ * Callback to run in fork()-ed process.
+ *
+ * Returns: 0 on success,
+ * -1 on error (treated as EXIT_CANCELED)
+ */
+typedef int (*virProcessForkCallback)(pid_t pid,
+ void *opaque);
+
+int virProcessRunInFork(virProcessForkCallback cb,
+ void *opaque);
+
int virProcessSetupPrivateMountNS(void);
int virProcessSetScheduler(pid_t pid,
--
2.18.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Oct 17, 2018 at 11:06:43AM +0200, Michal Privoznik wrote:
> This new helper can be used to spawn a child process and run
> passed callback from it. This will come handy esp. if the
> callback is not thread safe.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virprocess.c | 81 ++++++++++++++++++++++++++++++++++++++++
> src/util/virprocess.h | 16 ++++++++
> 3 files changed, 98 insertions(+)
> +/**
> + * virProcessRunInFork:
> + * @cb: callback to run
> + * @opaque: opaque data to @cb
> + *
> + * Do the fork and run @cb in the child. This can be used when
> + * @cb does something thread unsafe, for instance. However, due
> + * to possible signal propagation @cb should only call async
> + * signal safe functions. On return, the returned value is either
> + * -1 with error message reported if something went bad in the
> + * parent, if child has died from a signal or if the child
> + * returned EXIT_CANCELED. Otherwise the returned value is the
> + * exit status of the child.
Instead of;
However, due to possible signal propagation @cb should only call
async signal safe functions.
Use:
All signals will be reset to have their platform default handlers
and unmasked. @cb must only use async signal safe functions. In
particular no mutexes should be used in @cb, unless steps were
taken before forking to guarantee a predictable state. @cb must
not exec any external binaries, instead virCommand/virExec should
be used for that purpose.
> + */
> +int
> +virProcessRunInFork(virProcessForkCallback cb,
> + void *opaque)
> +{
> + int ret = -1;
> + pid_t child = -1;
> + pid_t parent = getpid();
> + int errfd[2] = { -1, -1 };
> +
> + if (pipe2(errfd, O_CLOEXEC) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot create pipe for child"));
> + return -1;
> + }
> +
> + if ((child = virFork()) < 0)
> + goto cleanup;
> +
> + if (child == 0) {
> + VIR_FORCE_CLOSE(errfd[0]);
> + ret = virProcessForkHelper(errfd[1], parent, cb, opaque);
> + VIR_FORCE_CLOSE(errfd[1]);
> + _exit(ret < 0 ? EXIT_CANCELED : ret);
> + } else {
> + int status;
> + VIR_AUTOFREE(char *) buf = NULL;
> +
> + VIR_FORCE_CLOSE(errfd[1]);
> + ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
> + ret = virProcessWait(child, &status, false);
> + if (!ret) {
> + ret = status == EXIT_CANCELED ? -1 : status;
> + if (ret) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("child reported: %s"),
> + NULLSTR(buf));
It would be desirable to include the exit status value in the message
> + }
> + }
> + }
> +
> + cleanup:
> + VIR_FORCE_CLOSE(errfd[0]);
> + VIR_FORCE_CLOSE(errfd[1]);
> + return ret;
> +}
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/17/18 5:06 AM, Michal Privoznik wrote:
> This new helper can be used to spawn a child process and run
> passed callback from it. This will come handy esp. if the
> callback is not thread safe.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virprocess.c | 81 ++++++++++++++++++++++++++++++++++++++++
> src/util/virprocess.h | 16 ++++++++
> 3 files changed, 98 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 335210c31d..e2dc85310a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2625,6 +2625,7 @@ virProcessKill;
> virProcessKillPainfully;
> virProcessKillPainfullyDelay;
> virProcessNamespaceAvailable;
> +virProcessRunInFork;
> virProcessRunInMountNamespace;
> virProcessSchedPolicyTypeFromString;
> virProcessSchedPolicyTypeToString;
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 3883c708fc..51b9ccb1bb 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -1165,6 +1165,87 @@ virProcessRunInMountNamespace(pid_t pid,
> }
>
>
> +static int
> +virProcessForkHelper(int errfd,
For consistency, virProcessRunInForkHelper
> + pid_t pid,
s/pid/parent (or ppid, IDC)
just to be clear
> + virProcessForkCallback cb,
> + void *opaque)
> +{
> + if (cb(pid, opaque) < 0) {
> + virErrorPtr err = virGetLastError();
> + if (err) {
> + size_t len = strlen(err->message) + 1;
> + ignore_value(safewrite(errfd, err->message, len));
> + }
> +
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> +/**
> + * virProcessRunInFork:
> + * @cb: callback to run
> + * @opaque: opaque data to @cb
> + *
> + * Do the fork and run @cb in the child. This can be used when
> + * @cb does something thread unsafe, for instance. However, due
> + * to possible signal propagation @cb should only call async
> + * signal safe functions. On return, the returned value is either
> + * -1 with error message reported if something went bad in the
> + * parent, if child has died from a signal or if the child
> + * returned EXIT_CANCELED. Otherwise the returned value is the
> + * exit status of the child.
> + */
> +int
> +virProcessRunInFork(virProcessForkCallback cb,
> + void *opaque)
> +{
> + int ret = -1;
> + pid_t child = -1;
> + pid_t parent = getpid();
> + int errfd[2] = { -1, -1 };
> +
> + if (pipe2(errfd, O_CLOEXEC) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot create pipe for child"));
> + return -1;
> + }
> +
> + if ((child = virFork()) < 0)
> + goto cleanup;
> +
> + if (child == 0) {
> + VIR_FORCE_CLOSE(errfd[0]);
> + ret = virProcessForkHelper(errfd[1], parent, cb, opaque);
> + VIR_FORCE_CLOSE(errfd[1]);
> + _exit(ret < 0 ? EXIT_CANCELED : ret);
> + } else {
> + int status;
> + VIR_AUTOFREE(char *) buf = NULL;
> +
> + VIR_FORCE_CLOSE(errfd[1]);
> + ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
> + ret = virProcessWait(child, &status, false);
> + if (!ret) {
<sigh>
> + ret = status == EXIT_CANCELED ? -1 : status;
> + if (ret) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("child reported: %s"),
> + NULLSTR(buf));
> + }
I agree w/ Daniel regarding @status reporting like virCommandWait, it
looks ugly but gets all the information out at least.
> + }
> + }
> +
> + cleanup:
> + VIR_FORCE_CLOSE(errfd[0]);
> + VIR_FORCE_CLOSE(errfd[1]);
> + return ret;
> +}
> +
> +
> #if defined(HAVE_SYS_MOUNT_H) && defined(HAVE_UNSHARE)
> int
> virProcessSetupPrivateMountNS(void)
> diff --git a/src/util/virprocess.h b/src/util/virprocess.h
> index 5faa0892fe..1bae8c9212 100644
> --- a/src/util/virprocess.h
> +++ b/src/util/virprocess.h
> @@ -93,6 +93,22 @@ int virProcessRunInMountNamespace(pid_t pid,
> virProcessNamespaceCallback cb,
> void *opaque);
>
> +/**
> + * virProcessForkCallback:
> + * @pid: parent's pid
It's the "fork's" parent pid. Perhaps would be nice to have ppid or
parent - just because pid is used so many times. It's not that important
though.
> + * @opaque: opaque data
> + *
> + * Callback to run in fork()-ed process.
> + *
> + * Returns: 0 on success,
> + * -1 on error (treated as EXIT_CANCELED)
> + */
> +typedef int (*virProcessForkCallback)(pid_t pid,
> + void *opaque);
> +
> +int virProcessRunInFork(virProcessForkCallback cb,
> + void *opaque);
> +
Since this is defined before virProcessRunInMountNamespace in source,
probably could do the same in this file. It's a type a orderly thing.
and yes, essentially what I was looking to see done from previous
reviews since those paths won't necessarily be NS type runs...
I'm OK with trusting that you can make the necessary comment adjustments
from Daniel's review, but if you feel compelled to post a diff I won't
complain either...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
> int virProcessSetupPrivateMountNS(void);
>
> int virProcessSetScheduler(pid_t pid,
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.