virFork: simplify semantics

The old semantics of virFork() violates the priciple of good
usability: it requires the caller to check the pid argument
after use, *even when virFork returned -1*, in order to properly
abort a child process that failed setup done immediately after
fork() - that is, the caller must call _exit() in the child.
While uses in virfile.c did this correctly, uses in 'virsh
lxc-enter-namespace' and 'virt-login-shell' would happily return
from the calling function in both the child and the parent,
leading to very confusing results. [Thankfully, I found the
problem by inspection, and can't actually trigger the double
return on error without an LD_PRELOAD library.]

It is much better if the semantics of virFork are impossible
to abuse.  Looking at virFork(), the parent could only ever
return -1 with a non-negative pid if it misused pthread_sigmask,
but this never happens.  Up until this patch series, the child
could return -1 with non-negative pid if it fails to set up
signals correctly, but we recently fixed that to make the child
call _exit() at that point instead of forcing the caller to do
it.  Thus, the return value and contents of the pid argument are
now redundant (a -1 return now happens only for failure to fork,
a child 0 return only happens for a successful 0 pid, and a
parent 0 return only happens for a successful non-zero pid),
so we might as well return the pid directly rather than an
integer of whether it succeeded or failed; this is also good
from the interface design perspective as users are already
familiar with fork() semantics.

One last change in this patch: before returning the pid directly,
I found cases where using virProcessWait unconditionally on a
cleanup path of a virFork's -1 pid return would be nicer if there
were a way to avoid it overwriting an earlier message.  While
such paths are a bit harder to come by with my change to a direct
pid return, I decided to keep the virProcessWait change in this
patch.

* src/util/vircommand.h (virFork): Change signature.
* src/util/vircommand.c (virFork): Guarantee that child will only
return on success, to simplify callers.  Return pid rather than
status, now that the situations are always the same.
(virExec): Adjust caller, also avoid open-coding process death.
* src/util/virprocess.c (virProcessWait): Tweak semantics when pid
is -1.
(virProcessRunInMountNamespace): Adjust caller.
* src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Likewise.
* tools/virt-login-shell.c (main): Likewise.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
* tests/commandtest.c (test23): Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>

src/util/vircommand.c

@@ -197,28 +197,28 @@ virCommandFDSet(virCommandPtr cmd,
 
 /**
  * virFork:
- * @pid - a pointer to a pid_t that will receive the return value from
- *        fork()
  *
  * Wrapper around fork() that avoids various race/deadlock conditions.
  *
- * on return from virFork(), if *pid < 0, the fork failed and there is
+ * Like fork(), there are several return possibilities:
- * no new process. Otherwise, just like fork(), if *pid == 0, it is the
+ * 1. No child was created: the return is -1, errno is set, and an error
- * child process returning, and if *pid > 0, it is the parent.
+ * message has been reported.  The semantics of virWaitProcess() recognize
- *
+ * this to avoid clobbering the error message from here.
- * Even if *pid >= 0, if the return value from virFork() is < 0, it
+ * 2. This is the parent: the return is > 0.  The parent can now attempt
- * indicates a failure that occurred in the parent or child process
+ * to interact with the child (but be aware that unlike raw fork(), the
- * after the fork. In this case, the child process should call
+ * child may not return - some failures in the child result in this
- * _exit(EXIT_CANCELED) after doing any additional error reporting.
+ * function calling _exit(EXIT_CANCELED) if the child cannot be set up
+ * correctly).
+ * 3. This is the child: the return is 0.  If this happens, the parent
+ * is also guaranteed to return.
  */
-int
+pid_t
-virFork(pid_t *pid)
+virFork(void)
 {
     sigset_t oldmask, newmask;
     struct sigaction sig_action;
-    int saved_errno, ret = -1;
+    int saved_errno;
-
+    pid_t pid;
-    *pid = -1;
 
     /*
      * Need to block signals now, so that child process can safely
@@ -226,54 +226,47 @@ virFork(pid_t *pid)
      */
     sigfillset(&newmask);
     if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) {
-        saved_errno = errno;
         virReportSystemError(errno,
                              "%s", _("cannot block signals"));
-        goto cleanup;
+        return -1;
     }
 
     /* Ensure we hold the logging lock, to protect child processes
      * from deadlocking on another thread's inherited mutex state */
     virLogLock();
 
-    *pid = fork();
+    pid = fork();
     saved_errno = errno; /* save for caller */
 
     /* Unlock for both parent and child process */
     virLogUnlock();
 
-    if (*pid < 0) {
+    if (pid < 0) {
         /* attempt to restore signal mask, but ignore failure, to
-           avoid obscuring the fork failure */
+         * avoid obscuring the fork failure */
         ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
         virReportSystemError(saved_errno,
                              "%s", _("cannot fork child process"));
-        goto cleanup;
+        errno = saved_errno;
-    }
-
-    if (*pid) {
 
+    } else if (pid) {
         /* parent process */
 
         /* Restore our original signal mask now that the child is
-           safely running */
+         * safely running. Only documented failures are EFAULT (not
-        if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) {
+         * possible, since we are using just-grabbed mask) or EINVAL
-            saved_errno = errno; /* save for caller */
+         * (not possible, since we are using correct arguments).  */
-            virReportSystemError(errno, "%s", _("cannot unblock signals"));
+        ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
-            goto cleanup;
-        }
-        ret = 0;
 
     } else {
-
         /* child process */
 
         int logprio;
         size_t i;
 
-        /* Remove any error callback so errors in child now
+        /* Remove any error callback so errors in child now get sent
-           get sent to stderr where they stand a fighting chance
+         * to stderr where they stand a fighting chance of being seen
-           of being seen / logged */
+         * and logged */
         virSetErrorFunc(NULL, NULL);
         virSetErrorLogPriorityFunc(NULL);
 
@@ -284,37 +277,30 @@ virFork(pid_t *pid)
         virLogSetDefaultPriority(logprio);
 
         /* Clear out all signal handlers from parent so nothing
-           unexpected can happen in our child once we unblock
+         * unexpected can happen in our child once we unblock
-           signals */
+         * signals */
         sig_action.sa_handler = SIG_DFL;
         sig_action.sa_flags = 0;
         sigemptyset(&sig_action.sa_mask);
 
         for (i = 1; i < NSIG; i++) {
-            /* Only possible errors are EFAULT or EINVAL
+            /* Only possible errors are EFAULT or EINVAL The former
-               The former wont happen, the latter we
+             * won't happen, the latter we expect, so no need to check
-               expect, so no need to check return value */
+             * return value */
-
+            ignore_value(sigaction(i, &sig_action, NULL));
-            sigaction(i, &sig_action, NULL);
         }
 
-        /* Unmask all signals in child, since we've no idea
+        /* Unmask all signals in child, since we've no idea what the
-           what the caller's done with their signal mask
+         * caller's done with their signal mask and don't want to
-           and don't want to propagate that to children */
+         * propagate that to children */
         sigemptyset(&newmask);
         if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) {
-            saved_errno = errno; /* save for caller */
             virReportSystemError(errno, "%s", _("cannot unblock signals"));
             virDispatchError(NULL);
             _exit(EXIT_CANCELED);
         }
-        ret = 0;
     }
-
+    return pid;
-cleanup:
-    if (ret < 0)
-        errno = saved_errno;
-    return ret;
 }
 
 /*
@@ -412,7 +398,7 @@ virExec(virCommandPtr cmd)
     int tmpfd;
     char *binarystr = NULL;
     const char *binary = NULL;
-    int forkRet, ret;
+    int ret;
     struct sigaction waxon, waxoff;
     gid_t *groups = NULL;
     int ngroups;
@@ -489,17 +475,13 @@ virExec(virCommandPtr cmd)
     if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
         goto cleanup;
 
-    forkRet = virFork(&pid);
+    pid = virFork();
 
     if (pid < 0) {
         goto cleanup;
     }
 
     if (pid) { /* parent */
-        if (forkRet < 0) {
-            goto cleanup;
-        }
-
         VIR_FORCE_CLOSE(null);
         if (cmd->outfdptr && *cmd->outfdptr == -1) {
             VIR_FORCE_CLOSE(pipeout[1]);
@@ -521,13 +503,6 @@ virExec(virCommandPtr cmd)
     /* child */
 
     ret = EXIT_CANCELED;
-    if (forkRet < 0) {
-        /* The fork was successful, but after that there was an error
-         * in the child (which was already logged).
-        */
-        goto fork_error;
-    }
-
     openmax = sysconf(_SC_OPEN_MAX);
     if (openmax < 0) {
         virReportSystemError(errno,  "%s",
@@ -598,9 +573,7 @@ virExec(virCommandPtr cmd)
 
         if (pid > 0) {
             if (cmd->pidfile && (virPidFileWritePath(cmd->pidfile, pid) < 0)) {
-                kill(pid, SIGTERM);
+                if (virProcessKillPainfully(pid, true) >= 0)
-                usleep(500*1000);
-                kill(pid, SIGTERM);
                     virReportSystemError(errno,
                                          _("could not write pidfile %s for %d"),
                                          cmd->pidfile, pid);
@@ -785,10 +758,9 @@ virExec(virCommandPtr cmd ATTRIBUTE_UNUSED)
     return -1;
 }
 
-int
+pid_t
-virFork(pid_t *pid)
+virFork(void)
 {
-    *pid = -1;
     errno = ENOTSUP;
 
     return -1;

src/util/vircommand.h

@@ -33,7 +33,7 @@ typedef virCommand *virCommandPtr;
  * call any function that is not async-signal-safe.  */
 typedef int (*virExecHook)(void *data);
 
-int virFork(pid_t *pid) ATTRIBUTE_RETURN_CHECK;
+pid_t virFork(void) ATTRIBUTE_RETURN_CHECK;
 
 int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK;
 

src/util/virfile.c

@@ -1730,7 +1730,7 @@ virFileAccessibleAs(const char *path, int mode,
     if (ngroups < 0)
         return -1;
 
-    forkRet = virFork(&pid);
+    pid = virFork();
 
     if (pid < 0) {
         VIR_FREE(groups);
@@ -1741,6 +1741,7 @@ virFileAccessibleAs(const char *path, int mode,
         VIR_FREE(groups);
         if (virProcessWait(pid, &status, false) < 0) {
             /* virProcessWait() already reported error */
+            errno = EINTR;
             return -1;
         }
 
@@ -1836,7 +1837,6 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
     int waitret, status, ret = 0;
     int fd = -1;
     int pair[2] = { -1, -1 };
-    int forkRet;
     gid_t *groups;
     int ngroups;
 
@@ -1858,7 +1858,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
         return ret;
     }
 
-    forkRet = virFork(&pid);
+    pid = virFork();
     if (pid < 0)
         return -errno;
 
@@ -1866,15 +1866,8 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
 
         /* child */
 
-        VIR_FORCE_CLOSE(pair[0]); /* preserves errno */
-        if (forkRet < 0) {
-            /* error encountered and logged in virFork() after the fork. */
-            ret = -errno;
-            goto childerror;
-        }
-
         /* set desired uid/gid, then attempt to create the file */
-
+        VIR_FORCE_CLOSE(pair[0]);
         if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
             ret = -errno;
             goto childerror;
@@ -2145,7 +2138,7 @@ virDirCreate(const char *path,
     if (ngroups < 0)
         return -errno;
 
-    int forkRet = virFork(&pid);
+    pid = virFork();
 
     if (pid < 0) {
         ret = -errno;
@@ -2175,13 +2168,7 @@ parenterror:
 
     /* child */
 
-    if (forkRet < 0) {
-        /* error encountered and logged in virFork() after the fork. */
-        goto childerror;
-    }
-
     /* set desired uid/gid, then attempt to create the directory */
-
     if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
         ret = -errno;
         goto childerror;

src/util/virprocess.c

@@ -157,12 +157,16 @@ virProcessAbort(pid_t pid)
  * @exitstatus: optional status collection
  * @raw: whether to pass non-normal status back to caller
  *
- * Wait for a child process to complete.  If @exitstatus is NULL, then the
+ * Wait for a child process to complete.  If @pid is -1, do nothing, but
- * child must exit normally with status 0.  Otherwise, if @raw is false,
+ * return -1 (useful for error cleanup, and assumes an earlier message was
- * the child must exit normally, and @exitstatus will contain the final
+ * already issued).  All other pids issue an error message on failure.
- * exit status (no need for the caller to use WEXITSTATUS()).  If @raw is
+ *
- * true, then the result of wait() is returned in @exitstatus, and the
+ * If @exitstatus is NULL, then the child must exit normally with status 0.
- * caller must use WIFEXITED() and friends to decipher the child's status.
+ * Otherwise, if @raw is false, the child must exit normally, and
+ * @exitstatus will contain the final exit status (no need for the caller
+ * to use WEXITSTATUS()).  If @raw is true, then the result of waitpid() is
+ * returned in @exitstatus, and the caller must use WIFEXITED() and friends
+ * to decipher the child's status.
  *
  * Returns 0 on a successful wait.  Returns -1 on any error waiting for
  * completion, or if the command completed with a status that cannot be
@@ -175,6 +179,7 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)
     int status;
 
     if (pid <= 0) {
+        if (pid != -1)
             virReportSystemError(EINVAL, _("unable to wait for process %lld"),
                                  (long long) pid);
         return -1;
@@ -956,16 +961,8 @@ virProcessRunInMountNamespace(pid_t pid,
         return -1;
     }
 
-    ret = virFork(&child);
+    if ((child = virFork()) < 0)
-
-    if (ret < 0 || child < 0) {
-        if (child == 0)
-            _exit(EXIT_CANCELED);
-
-        /* parent */
-        virProcessAbort(child);
         goto cleanup;
-    }
 
     if (child == 0) {
         VIR_FORCE_CLOSE(errfd[0]);

tests/commandtest.c

@@ -972,12 +972,10 @@ test23(const void *unused ATTRIBUTE_UNUSED)
     int status = -1;
     pid_t pid;
 
-    if (virFork(&pid) < 0)
+    if ((pid = virFork()) < 0)
-        goto cleanup;
-    if (pid < 0)
         goto cleanup;
     if (pid == 0) {
-        if (virFork(&pid) < 0)
+        if ((pid = virFork()) < 0)
             _exit(EXIT_FAILURE);
         if (pid == 0)
             _exit(42);
@@ -994,12 +992,10 @@ test23(const void *unused ATTRIBUTE_UNUSED)
         goto cleanup;
     }
 
-    if (virFork(&pid) < 0)
+    if ((pid = virFork()) < 0)
-        goto cleanup;
-    if (pid < 0)
         goto cleanup;
     if (pid == 0) {
-        if (virFork(&pid) < 0)
+        if ((pid = virFork()) < 0)
             _exit(EXIT_FAILURE);
         if (pid == 0) {
             raise(SIGKILL);

tools/virsh-domain.c

@@ -8177,9 +8177,10 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
     }
 
     /* Fork once because we don't want to affect
-     * virsh's namespace itself
+     * virsh's namespace itself, and because user namespace
+     * can only be changed in single-threaded process
      */
-    if (virFork(&pid) < 0)
+    if ((pid = virFork()) < 0)
         goto cleanup;
     if (pid == 0) {
         if (setlabel &&
@@ -8200,7 +8201,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
         /* Fork a second time because entering the
          * pid namespace only takes effect after fork
          */
-        if (virFork(&pid) < 0)
+        if ((pid = virFork()) < 0)
             _exit(255);
         if (pid == 0) {
             execv(cmdargv[0], cmdargv);

tools/virt-login-shell.c

@@ -309,7 +309,10 @@ main(int argc, char **argv)
     if (virDomainGetSecurityLabel(dom, seclabel) < 0)
         goto cleanup;
 
-    if (virFork(&cpid) < 0)
+    /* Fork once because we don't want to affect virt-login-shell's
+     * namespace itself.  FIXME: is this necessary?
+     */
+    if ((cpid = virFork()) < 0)
         goto cleanup;
 
     if (cpid == 0) {
@@ -318,9 +321,6 @@ main(int argc, char **argv)
         int openmax = sysconf(_SC_OPEN_MAX);
         int fd;
 
-        /* Fork once because we don't want to affect
-         * virt-login-shell's namespace itself
-         */
         if (virSetUIDGID(0, 0, NULL, 0) < 0)
             return EXIT_FAILURE;
 
@@ -355,7 +355,9 @@ main(int argc, char **argv)
             VIR_MASS_CLOSE(tmpfd);
         }
 
-        if (virFork(&ccpid) < 0)
+        /* A fork is required to create new process in correct pid
+         * namespace.  */
+        if ((ccpid = virFork()) < 0)
             return EXIT_FAILURE;
 
         if (ccpid == 0) {