While auditing all callers of virCommandRun, I noticed that nwfilter code never paid attention to commands with a non-zero status. In the cases where status was captured, either the callers required that the status was 0, or they discarded any failures from virCommandRun. Collecting status manually means that a non-zero child exit status is not logged, but I could not see the benefit in avoiding the logging in any command issued in the code. Therefore, it was simpler to just nuke the wasted effort of manually checking or ignoring non-zero status. While at it, I also noticed that ebiptablesRemoveRules would actually report success if the child process failed for a reason other than non-zero status, such as OOM. * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesExecCLI): Drop parameter. (ebtablesApplyBasicRules, ebtablesApplyDHCPOnlyRules) (ebtablesApplyDropAllRules, ebtablesCleanAll) (ebiptablesApplyNewRules, ebiptablesTearNewRules) (ebiptablesTearOldRules, ebiptablesAllTeardown) (ebiptablesDriverInitWithFirewallD) (ebiptablesDriverTestCLITools, ebiptablesDriverProbeStateMatch): Adjust all clients. (ebiptablesRemoveRules): Likewise, and fix return value on failure. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 89 ++++++++++++------------------- 1 file changed, 35 insertions(+), 54 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index dc651a2..002a844 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -1,7 +1,7 @@ /* * nwfilter_ebiptables_driver.c: driver for ebtables/iptables on tap devices * - * Copyright (C) 2011-2013 Red Hat, Inc. + * Copyright (C) 2011-2014 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corp. * Copyright (C) 2010-2012 Stefan Berger * @@ -2799,8 +2799,6 @@ ebiptablesDisplayRuleInstance(void *_inst) * ebiptablesExecCLI: * @buf : pointer to virBuffer containing the string with the commands to * execute. - * @status: Pointer to an integer for returning the WEXITSTATUS of the - * commands executed via the script the was run. * @outbuf: Optional pointer to a string that will hold the buffer with * output of the executed command. The actual buffer holding * the message will be newly allocated by this function and @@ -2815,15 +2813,11 @@ ebiptablesDisplayRuleInstance(void *_inst) * NULL, then the script must exit with status 0). */ static int -ebiptablesExecCLI(virBufferPtr buf, - int *status, char **outbuf) +ebiptablesExecCLI(virBufferPtr buf, char **outbuf) { int rc = -1; virCommandPtr cmd; - if (status) - *status = 0; - if (!virBufferError(buf) && !virBufferUse(buf)) return 0; @@ -2837,7 +2831,7 @@ ebiptablesExecCLI(virBufferPtr buf, virMutexLock(&execCLIMutex); - rc = virCommandRun(cmd, status); + rc = virCommandRun(cmd, NULL); virMutexUnlock(&execCLIMutex); @@ -3293,7 +3287,7 @@ ebtablesApplyBasicRules(const char *ifname, ebtablesLinkTmpRootChain(&buf, 1, ifname, 1); ebtablesRenameTmpRootChain(&buf, 1, ifname); - if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) + if (ebiptablesExecCLI(&buf, NULL) < 0) goto tear_down_tmpebchains; return 0; @@ -3441,7 +3435,7 @@ ebtablesApplyDHCPOnlyRules(const char *ifname, ebtablesRenameTmpRootChain(&buf, 0, ifname); } - if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) + if (ebiptablesExecCLI(&buf, NULL) < 0) goto tear_down_tmpebchains; return 0; @@ -3511,7 +3505,7 @@ ebtablesApplyDropAllRules(const char *ifname) ebtablesRenameTmpRootChain(&buf, 1, ifname); ebtablesRenameTmpRootChain(&buf, 0, ifname); - if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) + if (ebiptablesExecCLI(&buf, NULL) < 0) goto tear_down_tmpebchains; return 0; @@ -3537,7 +3531,6 @@ ebtablesRemoveBasicRules(const char *ifname) static int ebtablesCleanAll(const char *ifname) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int cli_status; if (!ebtables_cmd_path) return 0; @@ -3556,7 +3549,7 @@ static int ebtablesCleanAll(const char *ifname) ebtablesRemoveTmpRootChain(&buf, 1, ifname); ebtablesRemoveTmpRootChain(&buf, 0, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); return 0; } @@ -3704,7 +3697,6 @@ ebiptablesApplyNewRules(const char *ifname, void **_inst) { size_t i, j; - int cli_status; ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst; virBuffer buf = VIR_BUFFER_INITIALIZER; virHashTablePtr chains_in_set = virHashCreate(10, NULL); @@ -3752,7 +3744,7 @@ ebiptablesApplyNewRules(const char *ifname, ebtablesRemoveTmpSubChains(&buf, ifname); ebtablesRemoveTmpRootChain(&buf, 1, ifname); ebtablesRemoveTmpRootChain(&buf, 0, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); } NWFILTER_SET_EBTABLES_SHELLVAR(&buf); @@ -3771,7 +3763,7 @@ ebiptablesApplyNewRules(const char *ifname, qsort(&ebtChains[0], nEbtChains, sizeof(ebtChains[0]), ebiptablesRuleOrderSort); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpebchains; NWFILTER_SET_EBTABLES_SHELLVAR(&buf); @@ -3807,7 +3799,7 @@ ebiptablesApplyNewRules(const char *ifname, ebtChains[j++].commandTemplate, 'A', -1, 1); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpebchains; if (haveIptables) { @@ -3818,21 +3810,21 @@ ebiptablesApplyNewRules(const char *ifname, iptablesCreateBaseChains(&buf); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpebchains; NWFILTER_SET_IPTABLES_SHELLVAR(&buf); iptablesCreateTmpRootChains(&buf, ifname); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpiptchains; NWFILTER_SET_IPTABLES_SHELLVAR(&buf); iptablesLinkTmpRootChains(&buf, ifname); iptablesSetupVirtInPost(&buf, ifname); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpiptchains; NWFILTER_SET_IPTABLES_SHELLVAR(&buf); @@ -3845,7 +3837,7 @@ ebiptablesApplyNewRules(const char *ifname, 'A', -1, 1); } - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpiptchains; iptablesCheckBridgeNFCallEnabled(false); @@ -3859,21 +3851,21 @@ ebiptablesApplyNewRules(const char *ifname, iptablesCreateBaseChains(&buf); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpiptchains; NWFILTER_SET_IP6TABLES_SHELLVAR(&buf); iptablesCreateTmpRootChains(&buf, ifname); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpip6tchains; NWFILTER_SET_IP6TABLES_SHELLVAR(&buf); iptablesLinkTmpRootChains(&buf, ifname); iptablesSetupVirtInPost(&buf, ifname); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpip6tchains; NWFILTER_SET_IP6TABLES_SHELLVAR(&buf); @@ -3885,7 +3877,7 @@ ebiptablesApplyNewRules(const char *ifname, 'A', -1, 1); } - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_tmpip6tchains; iptablesCheckBridgeNFCallEnabled(true); @@ -3898,7 +3890,7 @@ ebiptablesApplyNewRules(const char *ifname, if (virHashSize(chains_out_set) != 0) ebtablesLinkTmpRootChain(&buf, 0, ifname, 1); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, &errmsg) < 0) goto tear_down_ebsubchains_and_unlink; virHashFree(chains_in_set); @@ -3945,7 +3937,7 @@ tear_down_tmpebchains: ebtablesRemoveTmpRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); virReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " @@ -3971,7 +3963,6 @@ exit_free_sets: static int ebiptablesTearNewRules(const char *ifname) { - int cli_status; virBuffer buf = VIR_BUFFER_INITIALIZER; if (iptables_cmd_path) { @@ -3999,7 +3990,7 @@ ebiptablesTearNewRules(const char *ifname) ebtablesRemoveTmpRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); return 0; } @@ -4008,7 +3999,6 @@ ebiptablesTearNewRules(const char *ifname) static int ebiptablesTearOldRules(const char *ifname) { - int cli_status; virBuffer buf = VIR_BUFFER_INITIALIZER; /* switch to new iptables user defined chains */ @@ -4019,7 +4009,7 @@ ebiptablesTearOldRules(const char *ifname) iptablesRemoveRootChains(&buf, ifname); iptablesRenameTmpRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); } if (ip6tables_cmd_path) { @@ -4029,7 +4019,7 @@ ebiptablesTearOldRules(const char *ifname) iptablesRemoveRootChains(&buf, ifname); iptablesRenameTmpRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); } if (ebtables_cmd_path) { @@ -4045,7 +4035,7 @@ ebiptablesTearOldRules(const char *ifname) ebtablesRenameTmpSubAndRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); } return 0; @@ -4068,8 +4058,7 @@ ebiptablesRemoveRules(const char *ifname ATTRIBUTE_UNUSED, int nruleInstances, void **_inst) { - int rc = 0; - int cli_status; + int rc = -1; size_t i; virBuffer buf = VIR_BUFFER_INITIALIZER; ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst; @@ -4082,17 +4071,12 @@ ebiptablesRemoveRules(const char *ifname ATTRIBUTE_UNUSED, 'D', -1, 0); - if (ebiptablesExecCLI(&buf, &cli_status, NULL) < 0) - goto err_exit; + if (ebiptablesExecCLI(&buf, NULL) < 0) + goto cleanup; - if (cli_status) { - virReportError(VIR_ERR_BUILD_FIREWALL, - "%s", - _("error while executing CLI commands")); - rc = -1; - } + rc = 0; -err_exit: +cleanup: return rc; } @@ -4110,7 +4094,6 @@ static int ebiptablesAllTeardown(const char *ifname) { virBuffer buf = VIR_BUFFER_INITIALIZER; - int cli_status; if (iptables_cmd_path) { NWFILTER_SET_IPTABLES_SHELLVAR(&buf); @@ -4139,7 +4122,7 @@ ebiptablesAllTeardown(const char *ifname) ebtablesRemoveRootChain(&buf, 1, ifname); ebtablesRemoveRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, NULL); return 0; } @@ -4180,7 +4163,6 @@ ebiptablesDriverInitWithFirewallD(void) virBuffer buf = VIR_BUFFER_INITIALIZER; char *firewall_cmd_path; char *output = NULL; - int status; int ret = -1; if (!virNWFilterDriverIsWatchingFirewallD()) @@ -4196,8 +4178,7 @@ ebiptablesDriverInitWithFirewallD(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, &status, &output) < 0 || - status != 0) { + if (ebiptablesExecCLI(&buf, &output) < 0) { VIR_INFO("firewalld support disabled for nwfilter"); } else { VIR_INFO("firewalld support enabled for nwfilter"); @@ -4268,7 +4249,7 @@ ebiptablesDriverTestCLITools(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) { + if (ebiptablesExecCLI(&buf, &errmsg) < 0) { VIR_FREE(ebtables_cmd_path); VIR_ERROR(_("Testing of ebtables command failed: %s"), errmsg); @@ -4285,7 +4266,7 @@ ebiptablesDriverTestCLITools(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) { + if (ebiptablesExecCLI(&buf, &errmsg) < 0) { VIR_FREE(iptables_cmd_path); VIR_ERROR(_("Testing of iptables command failed: %s"), errmsg); @@ -4302,7 +4283,7 @@ ebiptablesDriverTestCLITools(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) { + if (ebiptablesExecCLI(&buf, &errmsg) < 0) { VIR_FREE(ip6tables_cmd_path); VIR_ERROR(_("Testing of ip6tables command failed: %s"), errmsg); @@ -4353,7 +4334,7 @@ ebiptablesDriverProbeStateMatch(void) virBufferAsprintf(&buf, "$IPT --version"); - if (ebiptablesExecCLI(&buf, NULL, &cmdout) < 0) { + if (ebiptablesExecCLI(&buf, &cmdout) < 0) { VIR_ERROR(_("Testing of iptables command failed: %s"), cmdout); return; -- 1.8.5.3

Thanks to namespaces, we have a couple of places in the code base that want to reflect a child exit status, including the ability to detect death by a signal, back to a grandparent. Best to make it a reusable function. * src/util/virprocess.h (virProcessExitWithStatus): New prototype. * src/libvirt_private.syms (util/virprocess.h): Export it. * src/util/virprocess.c (virProcessExitWithStatus): New function. * tests/commandtest.c (test23): Test it. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 41 ++++++++++++++++++++++++++++++- src/util/virprocess.h | 4 ++- tests/commandtest.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ec786e4..d7a9ee7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1673,6 +1673,7 @@ virPortAllocatorRelease; # util/virprocess.h virProcessAbort; +virProcessExitWithStatus; virProcessGetAffinity; virProcessGetNamespaces; virProcessGetStartTime; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 305c095..68c4c14 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -1,7 +1,7 @@ /* * virprocess.c: interaction with processes * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -25,6 +25,7 @@ #include <fcntl.h> #include <signal.h> #include <errno.h> +#include <stdlib.h> #include <sys/wait.h> #if HAVE_SETRLIMIT # include <sys/time.h> @@ -983,3 +984,41 @@ virProcessRunInMountNamespace(pid_t pid ATTRIBUTE_UNUSED, return -1; } #endif + + +/** + * virProcessExitWithStatus: + * @status: raw status to be reproduced when this process dies + * + * Given a raw status obtained by waitpid() or similar, attempt to + * make this process exit in the same manner. If the child died by + * signal, reset that signal handler to default and raise the same + * signal; if that doesn't kill this process, then exit with 128 + + * signal number. If @status can't be deciphered, use + * EXIT_CANNOT_INVOKE. + * + * Never returns. + */ +void +virProcessExitWithStatus(int status) +{ + int value = EXIT_CANNOT_INVOKE; + + if (WIFEXITED(status)) { + value = WEXITSTATUS(status); + } else if (WIFSIGNALED(status)) { + struct sigaction act; + sigset_t sigs; + + if (sigemptyset(&sigs) == 0 && + sigaddset(&sigs, WTERMSIG(status)) == 0) + sigprocmask(SIG_UNBLOCK, &sigs, NULL); + memset(&act, 0, sizeof(act)); + act.sa_handler = SIG_DFL; + sigfillset(&act.sa_mask); + sigaction(WTERMSIG(status), &act, NULL); + raise(WTERMSIG(status)); + value = 128 + WTERMSIG(status); + } + exit(value); +} diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 5c173b0..b96dbd4 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -1,7 +1,7 @@ /* * virprocess.h: interaction with processes * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -33,6 +33,8 @@ virProcessTranslateStatus(int status); void virProcessAbort(pid_t pid); +void virProcessExitWithStatus(int status) ATTRIBUTE_NORETURN; + int virProcessWait(pid_t pid, int *exitstatus) ATTRIBUTE_RETURN_CHECK; diff --git a/tests/commandtest.c b/tests/commandtest.c index 042f049..fcda5e6 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -38,6 +38,7 @@ #include "virerror.h" #include "virthread.h" #include "virstring.h" +#include "virprocess.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -937,6 +938,68 @@ cleanup: return ret; } + +static int +test23(const void *unused ATTRIBUTE_UNUSED) +{ + /* Not strictly a virCommand test, but this is the easiest place + * to test this lower-level interface. It takes a double fork to + * test virProcessExitWithStatus. */ + int ret = -1; + int status = -1; + pid_t pid; + + if (virFork(&pid) < 0) + goto cleanup; + if (pid < 0) + goto cleanup; + if (pid == 0) { + if (virFork(&pid) < 0) + _exit(EXIT_FAILURE); + if (pid == 0) + _exit(42); + if (virProcessWait(pid, &status) < 0) + _exit(EXIT_FAILURE); + virProcessExitWithStatus(status); + _exit(EXIT_FAILURE); + } + + if (virProcessWait(pid, &status) < 0) + goto cleanup; + if (!WIFEXITED(status) || WEXITSTATUS(status) != 42) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + if (virFork(&pid) < 0) + goto cleanup; + if (pid < 0) + goto cleanup; + if (pid == 0) { + if (virFork(&pid) < 0) + _exit(EXIT_FAILURE); + if (pid == 0) { + raise(SIGKILL); + _exit(EXIT_FAILURE); + } + if (virProcessWait(pid, &status) < 0) + _exit(EXIT_FAILURE); + virProcessExitWithStatus(status); + _exit(EXIT_FAILURE); + } + + if (virProcessWait(pid, &status) < 0) + goto cleanup; + if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + ret = 0; +cleanup: + return ret; +} + static void virCommandThreadWorker(void *opaque) { virCommandTestDataPtr test = opaque; @@ -1085,6 +1148,7 @@ mymain(void) DO_TEST(test20); DO_TEST(test21); DO_TEST(test22); + DO_TEST(test23); virMutexLock(&test->lock); if (test->running) { -- 1.8.5.3

Right now, a caller waiting for a child process either requires the child to have status 0, or must use WIFEXITED() and friends itself. But in many cases, we want the middle ground of treating fatal signals as an error, and directly accessing the normal exit value without having to use WEXITSTATUS(), in order to easily detect an expected non-zero exit status. This adds the middle ground to the low-level virProcessWait; the next patch will add it to virCommand. * src/util/virprocess.h (virProcessWait): Alter signature. * src/util/virprocess.c (virProcessWait): Add parameter. (virProcessRunInMountNamespace): Adjust caller. * src/util/vircommand.c (virCommandWait): Likewise. * src/util/virfile.c (virFileAccessibleAs): Likewise. * src/lxc/lxc_container.c (lxcContainerHasReboot) (lxcContainerAvailable): Likewise. * daemon/libvirtd.c (daemonForkIntoBackground): Likewise. * tools/virt-login-shell.c (main): Likewise. * tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise. * tests/testutils.c (virtTestCaptureProgramOutput): Likewise. * tests/commandtest.c (test23): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- daemon/libvirtd.c | 4 ++-- src/lxc/lxc_container.c | 6 +++--- src/util/vircommand.c | 2 +- src/util/virfile.c | 14 ++++---------- src/util/virprocess.c | 46 ++++++++++++++++++++++++++++++---------------- src/util/virprocess.h | 2 +- tests/commandtest.c | 8 ++++---- tests/testutils.c | 4 ++-- tools/virsh-domain.c | 6 +++--- tools/virt-login-shell.c | 4 ++-- 10 files changed, 52 insertions(+), 44 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index b27c6fd..72f0e81 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1,7 +1,7 @@ /* * libvirtd.c: daemon start of day, guest process & i/o management * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -206,7 +206,7 @@ static int daemonForkIntoBackground(const char *argv0) VIR_FORCE_CLOSE(statuspipe[1]); /* We wait to make sure the first child forked successfully */ - if (virProcessWait(pid, NULL) < 0) + if (virProcessWait(pid, NULL, false) < 0) goto error; /* If we get here, then the grandchild was spawned, so we diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index f08dbc2..c63a470 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -173,11 +173,11 @@ int lxcContainerHasReboot(void) virReportSystemError(errno, "%s", _("Unable to clone to check reboot support")); return -1; - } else if (virProcessWait(cpid, &status) < 0) { + } else if (virProcessWait(cpid, &status, false) < 0) { return -1; } - if (WEXITSTATUS(status) != 1) { + if (status != 1) { VIR_DEBUG("Containerized reboot support is missing " "(kernel probably too old < 3.4)"); return 0; @@ -2075,7 +2075,7 @@ int lxcContainerAvailable(int features) VIR_DEBUG("clone call returned %s, container support is not enabled", virStrerror(errno, ebuf, sizeof(ebuf))); return -1; - } else if (virProcessWait(cpid, NULL) < 0) { + } else if (virProcessWait(cpid, NULL, false) < 0) { return -1; } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index b9e5f37..a4397b4 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2372,7 +2372,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) * message is not as detailed as what we can provide. So, we * guarantee that virProcessWait only fails due to failure to wait, * and repeat the exitstatus check code ourselves. */ - ret = virProcessWait(cmd->pid, exitstatus ? exitstatus : &status); + ret = virProcessWait(cmd->pid, exitstatus ? exitstatus : &status, true); if (cmd->flags & VIR_EXEC_ASYNC_IO) { cmd->flags &= ~VIR_EXEC_ASYNC_IO; virThreadJoin(cmd->asyncioThread); diff --git a/src/util/virfile.c b/src/util/virfile.c index 96f078d..6fb7d6f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1,7 +1,7 @@ /* * virfile.c: safer file handling * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2010 IBM Corporation * Copyright (C) 2010 Stefan Berger * Copyright (C) 2010 Eric Blake @@ -1739,19 +1739,13 @@ virFileAccessibleAs(const char *path, int mode, if (pid) { /* parent */ VIR_FREE(groups); - if (virProcessWait(pid, &status) < 0) { - /* virProcessWait() already - * reported error */ - return -1; - } - - if (!WIFEXITED(status)) { - errno = EINTR; + if (virProcessWait(pid, &status, false) < 0) { + /* virProcessWait() already reported error */ return -1; } if (status) { - errno = WEXITSTATUS(status); + errno = status; return -1; } diff --git a/src/util/virprocess.c b/src/util/virprocess.c index bd406db..72e9950 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -155,15 +155,21 @@ virProcessAbort(pid_t pid) * virProcessWait: * @pid: child to wait on * @exitstatus: optional status collection + * @raw: whether to pass non-normal status back to caller * - * 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. + * Wait for a child process to complete. If @exitstatus is NULL, then the + * child must exit normally with status 0. 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 wait() 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 + * reflected via the choice of @exitstatus and @raw. */ int -virProcessWait(pid_t pid, int *exitstatus) +virProcessWait(pid_t pid, int *exitstatus, bool raw) { int ret; int status; @@ -185,19 +191,27 @@ virProcessWait(pid_t pid, int *exitstatus) } if (exitstatus == NULL) { - if (status != 0) { - char *st = virProcessTranslateStatus(status); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Child process (%lld) unexpected %s"), - (long long) pid, NULLSTR(st)); - VIR_FREE(st); - return -1; - } - } else { + if (status != 0) + goto error; + } else if (raw) { *exitstatus = status; + } else if (WIFEXITED(status)) { + *exitstatus = WEXITSTATUS(status); + } else { + goto error; } return 0; + +error: + { + char *st = virProcessTranslateStatus(status); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Child process (%lld) unexpected %s"), + (long long) pid, NULLSTR(st)); + VIR_FREE(st); + } + return -1; } @@ -965,7 +979,7 @@ virProcessRunInMountNamespace(pid_t pid, VIR_FORCE_CLOSE(errfd[1]); ignore_value(virFileReadHeaderFD(errfd[0], 1024, &buf)); - ret = virProcessWait(child, &status); + ret = virProcessWait(child, &status, false); if (!ret) ret = status == EXIT_CANCELED ? -1 : status; VIR_FREE(buf); diff --git a/src/util/virprocess.h b/src/util/virprocess.h index abe3635..bcaede5 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -36,7 +36,7 @@ virProcessAbort(pid_t pid); void virProcessExitWithStatus(int status) ATTRIBUTE_NORETURN; int -virProcessWait(pid_t pid, int *exitstatus) +virProcessWait(pid_t pid, int *exitstatus, bool raw) ATTRIBUTE_RETURN_CHECK; int virProcessKill(pid_t pid, int sig); diff --git a/tests/commandtest.c b/tests/commandtest.c index fcda5e6..c0391a5 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -958,13 +958,13 @@ test23(const void *unused ATTRIBUTE_UNUSED) _exit(EXIT_FAILURE); if (pid == 0) _exit(42); - if (virProcessWait(pid, &status) < 0) + if (virProcessWait(pid, &status, true) < 0) _exit(EXIT_FAILURE); virProcessExitWithStatus(status); _exit(EXIT_FAILURE); } - if (virProcessWait(pid, &status) < 0) + if (virProcessWait(pid, &status, true) < 0) goto cleanup; if (!WIFEXITED(status) || WEXITSTATUS(status) != 42) { printf("Unexpected status %d\n", status); @@ -982,13 +982,13 @@ test23(const void *unused ATTRIBUTE_UNUSED) raise(SIGKILL); _exit(EXIT_FAILURE); } - if (virProcessWait(pid, &status) < 0) + if (virProcessWait(pid, &status, true) < 0) _exit(EXIT_FAILURE); virProcessExitWithStatus(status); _exit(EXIT_FAILURE); } - if (virProcessWait(pid, &status) < 0) + if (virProcessWait(pid, &status, true) < 0) goto cleanup; if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) { printf("Unexpected status %d\n", status); diff --git a/tests/testutils.c b/tests/testutils.c index 32fe374..018dd96 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -1,7 +1,7 @@ /* * testutils.c: basic test utils * - * Copyright (C) 2005-2013 Red Hat, Inc. + * Copyright (C) 2005-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -296,7 +296,7 @@ virtTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen) VIR_FORCE_CLOSE(pipefd[1]); len = virFileReadLimFD(pipefd[0], maxlen, buf); VIR_FORCE_CLOSE(pipefd[0]); - if (virProcessWait(pid, NULL) < 0) + if (virProcessWait(pid, NULL, false) < 0) return -1; return len; diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c3db94c..5cd3271 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1,7 +1,7 @@ /* * virsh-domain.c: Commands to manage domain * - * Copyright (C) 2005, 2007-2013 Red Hat, Inc. + * Copyright (C) 2005, 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -8227,7 +8227,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) execv(cmdargv[0], cmdargv); _exit(255); } else { - if (virProcessWait(pid, NULL) < 0) + if (virProcessWait(pid, NULL, false) < 0) _exit(255); } _exit(0); @@ -8235,7 +8235,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < nfdlist; i++) VIR_FORCE_CLOSE(fdlist[i]); VIR_FREE(fdlist); - if (virProcessWait(pid, NULL) < 0) + if (virProcessWait(pid, NULL, false) < 0) goto cleanup; } diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index 5b85d15..819cc5c 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -369,9 +369,9 @@ main(int argc, char **argv) return EXIT_FAILURE; } } - return virProcessWait(ccpid, &status2); + return virProcessWait(ccpid, &status2, true); } - ret = virProcessWait(cpid, &status); + ret = virProcessWait(cpid, &status, true); cleanup: virConfFree(conf); -- 1.8.5.3

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 | 120 ++++++++++++++++++----------------------------- src/util/vircommand.h | 2 +- src/util/virfile.c | 23 ++------- src/util/virprocess.c | 31 ++++++------ tests/commandtest.c | 12 ++--- tools/virsh-domain.c | 7 +-- tools/virt-login-shell.c | 12 +++-- 7 files changed, 81 insertions(+), 126 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 415b8c3..db4166f 100644 --- a/src/util/vircommand.c +++ b/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 - * 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_CANCELED) after doing any additional error reporting. + * 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(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 -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 @@ -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; - } - - 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); @@ -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 - 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")); virDispatchError(NULL); _exit(EXIT_CANCELED); } - ret = 0; } - -cleanup: - if (ret < 0) - errno = saved_errno; - return ret; + return pid; } /* @@ -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,12 +573,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(EXIT_SUCCESS); @@ -785,10 +758,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 ec6185d..7485edc 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 6fb7d6f..6da564b 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); @@ -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; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 72e9950..3620041 100644 --- a/src/util/virprocess.c +++ b/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 - * child must exit normally with status 0. 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 wait() is returned in @exitstatus, and the - * caller must use WIFEXITED() and friends to decipher the child's status. + * Wait for a child process to complete. If @pid is -1, do nothing, but + * return -1 (useful for error cleanup, and assumes an earlier message was + * already issued). All other pids issue an error message on failure. + * + * If @exitstatus is NULL, then the child must exit normally with status 0. + * 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,8 +179,9 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw) 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; } @@ -956,16 +961,8 @@ virProcessRunInMountNamespace(pid_t pid, return -1; } - ret = virFork(&child); - - if (ret < 0 || child < 0) { - if (child == 0) - _exit(EXIT_CANCELED); - - /* parent */ - virProcessAbort(child); + if ((child = virFork()) < 0) goto cleanup; - } if (child == 0) { VIR_FORCE_CLOSE(errfd[0]); diff --git a/tests/commandtest.c b/tests/commandtest.c index b329bfb..cb78a3c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -972,12 +972,10 @@ test23(const void *unused ATTRIBUTE_UNUSED) int status = -1; pid_t pid; - if (virFork(&pid) < 0) - goto cleanup; - if (pid < 0) + if ((pid = virFork()) < 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) - goto cleanup; - if (pid < 0) + if ((pid = virFork()) < 0) goto cleanup; if (pid == 0) { - if (virFork(&pid) < 0) + if ((pid = virFork()) < 0) _exit(EXIT_FAILURE); if (pid == 0) { raise(SIGKILL); diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5cd3271..4400d18 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8198,9 +8198,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 && @@ -8221,7 +8222,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 819cc5c..666facf 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.5.3

'virsh lxc-enter-namespace' does not have a way to reflect exit status to the caller in single-command mode, but we might as well at least report the exit status. Prior to this patch, $ virsh -c lxc:/// lxc-enter-namespace shell /bin/sh 'exit 3'; echo $? 1 now it gives some details: $ virsh -c lxc:/// lxc-enter-namespace shell /bin/sh -c 'exit 3'; echo $? error: internal error: Child process (31557) unexpected exit status 3 1 Also useful: $ virsh -c lxc:/// lxc-enter-namespace shell /bin/sh -c 'kill $$'; echo $? error: internal error: Child process (31585) unexpected fatal signal 15 1 * tools/virsh-domain.c (cmdLxcEnterNamespace): Avoid magic numbers. Dispatch any error. * tools/virsh.pod: Document that non-zero exit status is collapsed. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 21 ++++++++++++--------- tools/virsh.pod | 5 +++-- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4400d18..f9d85c3 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8204,12 +8204,14 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) if ((pid = virFork()) < 0) goto cleanup; if (pid == 0) { + int status; + if (setlabel && virDomainLxcEnterSecurityLabel(secmodel, seclabel, NULL, 0) < 0) - _exit(255); + _exit(EXIT_CANCELED); if (virDomainLxcEnterNamespace(dom, nfdlist, @@ -8217,27 +8219,28 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd) NULL, NULL, 0) < 0) - _exit(255); + _exit(EXIT_CANCELED); /* Fork a second time because entering the * pid namespace only takes effect after fork */ if ((pid = virFork()) < 0) - _exit(255); + _exit(EXIT_CANCELED); if (pid == 0) { execv(cmdargv[0], cmdargv); - _exit(255); - } else { - if (virProcessWait(pid, NULL, false) < 0) - _exit(255); + _exit(errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE); } - _exit(0); + if (virProcessWait(pid, &status, true) < 0) + _exit(EXIT_CANNOT_INVOKE); + virProcessExitWithStatus(status); } else { for (i = 0; i < nfdlist; i++) VIR_FORCE_CLOSE(fdlist[i]); VIR_FREE(fdlist); - if (virProcessWait(pid, NULL, false) < 0) + if (virProcessWait(pid, NULL, false) < 0) { + vshReportError(ctl); goto cleanup; + } } ret = true; diff --git a/tools/virsh.pod b/tools/virsh.pod index f221475..49e1f63 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3332,7 +3332,8 @@ Enter the namespace of I<domain> and execute the command C</path/to/binary> passing the requested args. The binary path is relative to the container root filesystem, not the host root filesystem. The binary will inherit the environment variables / console visible to virsh. This command only works -when connected to the LXC hypervisor driver. +when connected to the LXC hypervisor driver. This command succeeds only +if C</path/to/binary> has 0 exit status. =back @@ -3447,7 +3448,7 @@ Alternatively report bugs to your software distributor / vendor. =head1 COPYRIGHT -Copyright (C) 2005, 2007-2010 Red Hat, Inc., and the authors listed in the +Copyright (C) 2005, 2007-2014 Red Hat, Inc., and the authors listed in the libvirt AUTHORS file. =head1 LICENSE -- 1.8.5.3