[libvirt] [PATCH v3 01/13] virprocess: Introduce virProcessRunInFork

Michal Privoznik posted 13 patches 6 years, 6 months ago
[libvirt] [PATCH v3 01/13] virprocess: Introduce virProcessRunInFork
Posted by Michal Privoznik 6 years, 6 months ago
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
Re: [libvirt] [PATCH v3 01/13] virprocess: Introduce virProcessRunInFork
Posted by Daniel P. Berrangé 6 years, 6 months ago
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