From 8b24a803adcdbda11b0e080cb1dc3d13c83cd0d0 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Wed, 19 Feb 2014 12:39:44 -0700
Subject: [PATCH] util: preserve exit status from mount namespace callback

The documentation of namespace callbacks was inconsistent on whether
it preserved positive return values.  Now that we have a dedicated
EXIT_CANCELED to flag all errors before getting to the callback,
it is possible to use positive return values (not that any of the
current callers do, but it is better to match the docs).

Also, while vircommand.c is careful to close fds that a child should
not have, it's still better to be in the practice of setting
FD_CLOEXEC up front.

* src/util/virprocess.c (virProcessRunInMountNamespace): Tweak
return value to pass back non-zero status.  Avoid leaking pipe fds
to other threads.
* src/util/virprocess.h: Fix comment.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/util/virprocess.c | 19 +++++++++++--------
 src/util/virprocess.h |  2 +-
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 68c4c148dd..bd406db306 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -923,9 +923,9 @@ static int virProcessNamespaceHelper(int errfd,
 
 /* Run cb(opaque) in the mount namespace of pid.  Return -1 with error
  * message raised if we fail to run the child, if the child dies from
- * a signal, or if the child has status 1; otherwise return the exit
- * status of the child. The callback will be run in a child process
- * so must be careful to only use async signal safe functions.
+ * a signal, or if the child has status EXIT_CANCELED; otherwise return
+ * the exit status of the child. The callback will be run in a child
+ * process so must be careful to only use async signal safe functions.
  */
 int
 virProcessRunInMountNamespace(pid_t pid,
@@ -936,7 +936,7 @@ virProcessRunInMountNamespace(pid_t pid,
     pid_t child = -1;
     int errfd[2] = { -1, -1 };
 
-    if (pipe(errfd) < 0) {
+    if (pipe2(errfd, O_CLOEXEC) < 0) {
         virReportSystemError(errno, "%s",
                              _("Cannot create pipe for child"));
         return -1;
@@ -946,7 +946,7 @@ virProcessRunInMountNamespace(pid_t pid,
 
     if (ret < 0 || child < 0) {
         if (child == 0)
-            _exit(1);
+            _exit(EXIT_CANCELED);
 
         /* parent */
         virProcessAbort(child);
@@ -958,13 +958,16 @@ virProcessRunInMountNamespace(pid_t pid,
         ret = virProcessNamespaceHelper(errfd[1], pid,
                                         cb, opaque);
         VIR_FORCE_CLOSE(errfd[1]);
-        _exit(ret < 0 ? 1 : 0);
+        _exit(ret < 0 ? EXIT_CANCELED : ret);
     } else {
         char *buf = NULL;
-        VIR_FORCE_CLOSE(errfd[1]);
+        int status;
 
+        VIR_FORCE_CLOSE(errfd[1]);
         ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf));
-        ret = virProcessWait(child, NULL);
+        ret = virProcessWait(child, &status);
+        if (!ret)
+            ret = status == EXIT_CANCELED ? -1 : status;
         VIR_FREE(buf);
     }
 
diff --git a/src/util/virprocess.h b/src/util/virprocess.h
index b96dbd4210..abe3635bc9 100644
--- a/src/util/virprocess.h
+++ b/src/util/virprocess.h
@@ -67,7 +67,7 @@ int virProcessSetMaxFiles(pid_t pid, unsigned int files);
  * pid.  This function must use only async-signal-safe functions, as
  * it gets run after a fork of a multi-threaded process.  The return
  * value of this function is passed to _exit(), except that a
- * negative value is treated as an error.  */
+ * negative value is treated as EXIT_CANCELED.  */
 typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque);
 
 int virProcessRunInMountNamespace(pid_t pid,
-- 
GitLab