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>

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 68c4c148dd3ba55d89c0d5301d11102e7b09153c..bd406db3065081125000b5dde283483c62a2aab2 100644 (file)
--- 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 b96dbd4210208e214d50b35ef44377593f552c15..abe3635bc99c1ee9e264df747146ba2fbf8f4624 100644 (file)
--- 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,