[libvirt] [PATCHv2 2/7] virFork: simplify semantics
Eric Blake
eblake at redhat.com
Tue Dec 24 05:55:46 UTC 2013
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. The child could return -1 with
non-negative pid if it fails to set up signals correctly, which
is less likely but harder to guarantee - but if we instead make
the child call _exit() at that point instead of forcing the
caller to do it, then it is harder for the caller to misuse the
virFork interface. Note that once we make that change, the
semantics of the return value and 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/virprocess.c (virProcessWait): Tweak semantics when pid
is -1.
* 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/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Adjust callers.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
src/util/vircommand.c | 127 ++++++++++++++++++-----------------------------
src/util/vircommand.h | 2 +-
src/util/virfile.c | 25 ++--------
src/util/virprocess.c | 15 +++---
tools/virsh-domain.c | 7 +--
tools/virt-login-shell.c | 12 +++--
6 files changed, 75 insertions(+), 113 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index a52a1ab..ad767d7 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -193,28 +193,27 @@ virCommandFDSet(virCommandPtr cmd,
/**
* virFork:
- * @pid - a pointer to a pid_t that will receive the return value from
- * fork()
*
- * fork a new process while avoiding various race/deadlock conditions
- *
- * on return from virFork(), if *pid < 0, the fork failed and there is
- * no new process. Otherwise, just like fork(), if *pid == 0, it is the
- * child process returning, and if *pid > 0, it is the parent.
- *
- * Even if *pid >= 0, if the return value from virFork() is < 0, it
- * indicates a failure that occurred in the parent or child process
- * after the fork. In this case, the child process should call
- * _exit(EXIT_FAILURE) after doing any additional error reporting.
+ * Wrapper around fork() that avoids various race/deadlock conditions.
+ *
+ * Like fork(), there are several return possibilities:
+ * 1. No child was created: the return is -1, errno is set, and an error
+ * message has been reported. The semantics of virWaitProcess() recognize
+ * this to avoid clobbering the error message from here.
+ * 2. This is the parent: the return is > 0. The parent can now attempt
+ * to interact with the child (but be aware that unlike raw fork(), the
+ * child may not return - some failures in the child result in this
+ * function calling _exit(255) 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
-virFork(pid_t *pid)
+pid_t
+virFork(void)
{
sigset_t oldmask, newmask;
struct sigaction sig_action;
- int saved_errno, ret = -1;
-
- *pid = -1;
+ int saved_errno;
+ pid_t pid;
/*
* Need to block signals now, so that child process can safely
@@ -222,54 +221,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;
- }
-
- if (*pid) {
+ errno = saved_errno;
+ } else if (pid) {
/* parent process */
/* Restore our original signal mask now that the child is
- safely running */
- if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) {
- saved_errno = errno; /* save for caller */
- virReportSystemError(errno, "%s", _("cannot unblock signals"));
- goto cleanup;
- }
- ret = 0;
+ * safely running. Only documented failures are EFAULT (not
+ * possible, since we are using just-grabbed mask) or EINVAL
+ * (not possible, since we are using correct arguments). */
+ ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
} else {
-
/* child process */
int logprio;
size_t i;
- /* Remove any error callback so errors in child now
- get sent to stderr where they stand a fighting chance
- of being seen / logged */
+ /* Remove any error callback so errors in child now get sent
+ * to stderr where they stand a fighting chance of being seen
+ * and logged */
virSetErrorFunc(NULL, NULL);
virSetErrorLogPriorityFunc(NULL);
@@ -280,36 +272,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
- signals */
+ * unexpected can happen in our child once we unblock
+ * 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
- The former wont happen, the latter we
- expect, so no need to check return value */
-
- sigaction(i, &sig_action, NULL);
+ /* Only possible errors are EFAULT or EINVAL The former
+ * won't happen, the latter we expect, so no need to check
+ * return value */
+ ignore_value(sigaction(i, &sig_action, NULL));
}
- /* Unmask all signals in child, since we've no idea
- what the caller's done with their signal mask
- and don't want to propagate that to children */
+ /* Unmask all signals in child, since we've no idea what the
+ * caller's done with their signal mask and don't want to
+ * 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"));
- goto cleanup;
+ virDispatchError(NULL);
+ _exit(255);
}
- ret = 0;
}
-
-cleanup:
- if (ret < 0)
- errno = saved_errno;
- return ret;
+ return pid;
}
/*
@@ -407,7 +393,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;
@@ -484,17 +470,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]);
@@ -514,14 +496,6 @@ virExec(virCommandPtr cmd)
}
/* child */
-
- 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",
@@ -592,12 +566,10 @@ virExec(virCommandPtr cmd)
if (pid > 0) {
if (cmd->pidfile && (virPidFileWritePath(cmd->pidfile, pid) < 0)) {
- kill(pid, SIGTERM);
- usleep(500*1000);
- kill(pid, SIGTERM);
- virReportSystemError(errno,
- _("could not write pidfile %s for %d"),
- cmd->pidfile, pid);
+ if (virProcessKillPainfully(pid, true) >= 0)
+ virReportSystemError(errno,
+ _("could not write pidfile %s for %d"),
+ cmd->pidfile, pid);
goto fork_error;
}
_exit(0);
@@ -778,10 +750,9 @@ virExec(virCommandPtr cmd ATTRIBUTE_UNUSED)
return -1;
}
-int
-virFork(pid_t *pid)
+pid_t
+virFork(void)
{
- *pid = -1;
errno = ENOTSUP;
return -1;
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index e977f93..4372a7f 100644
--- a/src/util/vircommand.h
+++ b/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;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 631cd06..92d4815 100644
--- a/src/util/virfile.c
+++ b/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);
@@ -1740,8 +1740,7 @@ virFileAccessibleAs(const char *path, int mode,
if (pid) { /* parent */
VIR_FREE(groups);
if (virProcessWait(pid, &status) < 0) {
- /* virProcessWait() already
- * reported error */
+ /* virProcessWait() already reported error */
return -1;
}
@@ -1842,7 +1841,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;
@@ -1864,7 +1862,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
return ret;
}
- forkRet = virFork(&pid);
+ pid = virFork();
if (pid < 0)
return -errno;
@@ -1872,15 +1870,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;
@@ -2151,7 +2142,7 @@ virDirCreate(const char *path,
if (ngroups < 0)
return -errno;
- int forkRet = virFork(&pid);
+ pid = virFork();
if (pid < 0) {
ret = -errno;
@@ -2181,13 +2172,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;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 9fc3207..5fbed25 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -151,10 +151,12 @@ virProcessAbort(pid_t pid)
* @exitstatus: optional status collection
*
* Wait for a child process to complete.
- * Return -1 on any error waiting for
- * completion. Returns 0 if the command
- * finished with the exit status set. If @exitstatus is NULL, then the
- * child must exit with status 0 for this to succeed.
+ * If @pid is -1, do nothing, but return -1 (useful for error cleanup,
+ * and assumes an earlier message was already issued). Otherwise,
+ * return -1 on any error waiting for completion, with an error message
+ * issued. If @exitstatus is NULL, require the process to complete normally
+ * with status 0; otherwise, populate @exitstatus (whether the process
+ * completed normally or was terminated by a signal).
*/
int
virProcessWait(pid_t pid, int *exitstatus)
@@ -163,8 +165,9 @@ virProcessWait(pid_t pid, int *exitstatus)
int status;
if (pid <= 0) {
- virReportSystemError(EINVAL, _("unable to wait for process %lld"),
- (long long) pid);
+ if (pid != -1)
+ virReportSystemError(EINVAL, _("unable to wait for process %lld"),
+ (long long) pid);
return -1;
}
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 760dca5..d9b542e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8134,9 +8134,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 &&
@@ -8157,7 +8158,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);
diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
index ec78fbc..e26b7a1 100644
--- a/tools/virt-login-shell.c
+++ b/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) {
--
1.8.4.2
More information about the libvir-list
mailing list