On 02/20/2014 07:13 AM, Eric Blake wrote: You need to be careful with this - for some of the external execs in nwfilter (same with viriptables.c), a non-0 status *should* be ignored and not reported. In particular, if a command that is attempting to remove an iptables or ebtables rule fails, that is often because libvirt is attempting to remove a rule that actually isn't there. As a matter of fact, in all the cases where the 2nd argument to ebiptablesExecCLI is non-NULL, that is exactly what's happening - the code was written that way to avoid putting a bogus and misleading error message in the logs; viriptables.c *does* log these errors, and that has led to many bug reports that incorrectly list the error message about failure to remove a rule as evidence that there is a bug. (I think there may even be a BZ filed requesting that these error logs be removed because they are misleading.) Because of the experience with viriptables.c, I don't think we should change the code to add back in the logging of these messages.

While auditing all callers of virCommandRun, I noticed that nwfilter code never paid attention to commands with a non-zero status; they were merely passing a pointer to avoid spamming the logs with a message about commands that might indeed fail. But proving this required chasing through a lot of code; refactoring things to localize the decision of whether to ignore non-zero status makes it easier to prove that later changes to virFork don't negatively affect this code. 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): Change parameter from pointer to bool. (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(a)redhat.com&gt; --- v2 of this patch, the remaining 9 patches of the series are unchanged from v1 (other than rebasing), and I still think this is probably worth including in the 1.2.2 release. src/nwfilter/nwfilter_ebiptables_driver.c | 99 +++++++++++++------------------ 1 file changed, 41 insertions(+), 58 deletions(-) diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index dc651a2..953c08a 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 * @@ -2797,10 +2797,9 @@ 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. + * @buf: pointer to virBuffer containing the string with the commands to + * execute. + * @ignoreNonzero: true if non-zero status is not fatal * @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 @@ -2811,18 +2810,15 @@ ebiptablesDisplayRuleInstance(void *_inst) * script. * * Execute a sequence of commands (held in the given buffer) as a /bin/sh - * script and return the status of the execution in *status (if status is - * NULL, then the script must exit with status 0). + * script. Depending on ignoreNonzero, this function will fail if the + * script has unexpected status. */ static int -ebiptablesExecCLI(virBufferPtr buf, - int *status, char **outbuf) +ebiptablesExecCLI(virBufferPtr buf, bool ignoreNonzero, char **outbuf) { int rc = -1; virCommandPtr cmd; - - if (status) - *status = 0; + int status; if (!virBufferError(buf) && !virBufferUse(buf)) return 0; @@ -2837,7 +2833,7 @@ ebiptablesExecCLI(virBufferPtr buf, virMutexLock(&execCLIMutex); - rc = virCommandRun(cmd, status); + rc = virCommandRun(cmd, ignoreNonzero ? &status : NULL); virMutexUnlock(&execCLIMutex); @@ -3293,7 +3289,7 @@ ebtablesApplyBasicRules(const char *ifname, ebtablesLinkTmpRootChain(&buf, 1, ifname, 1); ebtablesRenameTmpRootChain(&buf, 1, ifname); - if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) + if (ebiptablesExecCLI(&buf, false, NULL) < 0) goto tear_down_tmpebchains; return 0; @@ -3441,7 +3437,7 @@ ebtablesApplyDHCPOnlyRules(const char *ifname, ebtablesRenameTmpRootChain(&buf, 0, ifname); } - if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) + if (ebiptablesExecCLI(&buf, false, NULL) < 0) goto tear_down_tmpebchains; return 0; @@ -3511,7 +3507,7 @@ ebtablesApplyDropAllRules(const char *ifname) ebtablesRenameTmpRootChain(&buf, 1, ifname); ebtablesRenameTmpRootChain(&buf, 0, ifname); - if (ebiptablesExecCLI(&buf, NULL, NULL) < 0) + if (ebiptablesExecCLI(&buf, false, NULL) < 0) goto tear_down_tmpebchains; return 0; @@ -3537,7 +3533,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 +3551,7 @@ static int ebtablesCleanAll(const char *ifname) ebtablesRemoveTmpRootChain(&buf, 1, ifname); ebtablesRemoveTmpRootChain(&buf, 0, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); return 0; } @@ -3704,7 +3699,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 +3746,7 @@ ebiptablesApplyNewRules(const char *ifname, ebtablesRemoveTmpSubChains(&buf, ifname); ebtablesRemoveTmpRootChain(&buf, 1, ifname); ebtablesRemoveTmpRootChain(&buf, 0, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); } NWFILTER_SET_EBTABLES_SHELLVAR(&buf); @@ -3771,7 +3765,7 @@ ebiptablesApplyNewRules(const char *ifname, qsort(&ebtChains[0], nEbtChains, sizeof(ebtChains[0]), ebiptablesRuleOrderSort); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) goto tear_down_tmpebchains; NWFILTER_SET_EBTABLES_SHELLVAR(&buf); @@ -3807,7 +3801,7 @@ ebiptablesApplyNewRules(const char *ifname, ebtChains[j++].commandTemplate, 'A', -1, 1); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) goto tear_down_tmpebchains; if (haveIptables) { @@ -3818,21 +3812,21 @@ ebiptablesApplyNewRules(const char *ifname, iptablesCreateBaseChains(&buf); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) goto tear_down_tmpebchains; NWFILTER_SET_IPTABLES_SHELLVAR(&buf); iptablesCreateTmpRootChains(&buf, ifname); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &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, false, &errmsg) < 0) goto tear_down_tmpiptchains; NWFILTER_SET_IPTABLES_SHELLVAR(&buf); @@ -3845,7 +3839,7 @@ ebiptablesApplyNewRules(const char *ifname, 'A', -1, 1); } - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) goto tear_down_tmpiptchains; iptablesCheckBridgeNFCallEnabled(false); @@ -3859,21 +3853,21 @@ ebiptablesApplyNewRules(const char *ifname, iptablesCreateBaseChains(&buf); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) goto tear_down_tmpiptchains; NWFILTER_SET_IP6TABLES_SHELLVAR(&buf); iptablesCreateTmpRootChains(&buf, ifname); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &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, false, &errmsg) < 0) goto tear_down_tmpip6tchains; NWFILTER_SET_IP6TABLES_SHELLVAR(&buf); @@ -3885,7 +3879,7 @@ ebiptablesApplyNewRules(const char *ifname, 'A', -1, 1); } - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) goto tear_down_tmpip6tchains; iptablesCheckBridgeNFCallEnabled(true); @@ -3898,7 +3892,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, false, &errmsg) < 0) goto tear_down_ebsubchains_and_unlink; virHashFree(chains_in_set); @@ -3945,7 +3939,7 @@ tear_down_tmpebchains: ebtablesRemoveTmpRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); virReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " @@ -3971,7 +3965,6 @@ exit_free_sets: static int ebiptablesTearNewRules(const char *ifname) { - int cli_status; virBuffer buf = VIR_BUFFER_INITIALIZER; if (iptables_cmd_path) { @@ -3999,7 +3992,7 @@ ebiptablesTearNewRules(const char *ifname) ebtablesRemoveTmpRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); return 0; } @@ -4008,7 +4001,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 +4011,7 @@ ebiptablesTearOldRules(const char *ifname) iptablesRemoveRootChains(&buf, ifname); iptablesRenameTmpRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); } if (ip6tables_cmd_path) { @@ -4029,7 +4021,7 @@ ebiptablesTearOldRules(const char *ifname) iptablesRemoveRootChains(&buf, ifname); iptablesRenameTmpRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); } if (ebtables_cmd_path) { @@ -4045,7 +4037,7 @@ ebiptablesTearOldRules(const char *ifname) ebtablesRenameTmpSubAndRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); } return 0; @@ -4068,8 +4060,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 +4073,12 @@ ebiptablesRemoveRules(const char *ifname ATTRIBUTE_UNUSED, 'D', -1, 0); - if (ebiptablesExecCLI(&buf, &cli_status, NULL) < 0) - goto err_exit; + if (ebiptablesExecCLI(&buf, true, 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 +4096,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 +4124,7 @@ ebiptablesAllTeardown(const char *ifname) ebtablesRemoveRootChain(&buf, 1, ifname); ebtablesRemoveRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status, NULL); + ebiptablesExecCLI(&buf, true, NULL); return 0; } @@ -4180,7 +4165,6 @@ ebiptablesDriverInitWithFirewallD(void) virBuffer buf = VIR_BUFFER_INITIALIZER; char *firewall_cmd_path; char *output = NULL; - int status; int ret = -1; if (!virNWFilterDriverIsWatchingFirewallD()) @@ -4196,8 +4180,7 @@ ebiptablesDriverInitWithFirewallD(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, &status, &output) < 0 || - status != 0) { + if (ebiptablesExecCLI(&buf, false, &output) < 0) { VIR_INFO("firewalld support disabled for nwfilter"); } else { VIR_INFO("firewalld support enabled for nwfilter"); @@ -4268,7 +4251,7 @@ ebiptablesDriverTestCLITools(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) { + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) { VIR_FREE(ebtables_cmd_path); VIR_ERROR(_("Testing of ebtables command failed: %s"), errmsg); @@ -4285,7 +4268,7 @@ ebiptablesDriverTestCLITools(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) { + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) { VIR_FREE(iptables_cmd_path); VIR_ERROR(_("Testing of iptables command failed: %s"), errmsg); @@ -4302,7 +4285,7 @@ ebiptablesDriverTestCLITools(void) "%s", CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, NULL, &errmsg) < 0) { + if (ebiptablesExecCLI(&buf, false, &errmsg) < 0) { VIR_FREE(ip6tables_cmd_path); VIR_ERROR(_("Testing of ip6tables command failed: %s"), errmsg); @@ -4353,7 +4336,7 @@ ebiptablesDriverProbeStateMatch(void) virBufferAsprintf(&buf, "$IPT --version"); - if (ebiptablesExecCLI(&buf, NULL, &cmdout) < 0) { + if (ebiptablesExecCLI(&buf, false, &cmdout) < 0) { VIR_ERROR(_("Testing of iptables command failed: %s"), cmdout); return; -- 1.8.5.3

When a child fails without exec'ing, we want a well-known status; best is to match what env(1), nice(1), su(1), and other wrapper programs do. This patch adds enum values that later patches will use, and sets up virFork as the first client of EXIT_CANCELED for errors detected prior to even attempting exec, as well as virExec to distinguish between a missing executable vs. a binary that cannot be executed. This is a slight semantic change in the unlikely case of a child process failing to restore its signal mask - we now kill the child with a known status instead of relying on the caller to notice and do an appropriate _exit(). A subsequent patch will make further cleanups based on an audit of all callers. * src/internal.h (EXIT_CANCELED, EXIT_CANNOT_INVOKE) (EXIT_ENOENT): New enum. * src/util/vircommand.c (virFork): Document specific exit value if child aborts early. (virExec): Distinguish between various exec failures. * tests/commandtest.c (test1): Enhance test. (test22): New test. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/internal.h | 7 +++++++ src/util/vircommand.c | 15 +++++++++------ tests/commandtest.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/internal.h b/src/internal.h index cef3da0..5a38448 100644 --- a/src/internal.h +++ b/src/internal.h @@ -438,5 +438,12 @@ #NAME ": " FMT, __VA_ARGS__); # endif +/* Specific error values for use in forwarding programs such as + * virt-login-shell; these values match what GNU env does. */ +enum { + EXIT_CANCELED = 125, /* Failed before attempting exec */ + EXIT_CANNOT_INVOKE = 126, /* Exists but couldn't exec */ + EXIT_ENOENT = 127, /* Could not find program to exec */ +}; #endif /* __VIR_INTERNAL_H__ */ diff --git a/src/util/vircommand.c b/src/util/vircommand.c index b137436..b9e5f37 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1,7 +1,7 @@ /* * vircommand.c: Child command execution * - * 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 @@ -199,7 +199,7 @@ virCommandFDSet(virCommandPtr cmd, * @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 + * 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 @@ -208,7 +208,7 @@ virCommandFDSet(virCommandPtr cmd, * 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. + * _exit(EXIT_CANCELED) after doing any additional error reporting. */ int virFork(pid_t *pid) @@ -304,7 +304,8 @@ virFork(pid_t *pid) 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(EXIT_CANCELED); } ret = 0; } @@ -518,6 +519,7 @@ 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). @@ -603,7 +605,7 @@ virExec(virCommandPtr cmd) cmd->pidfile, pid); goto fork_error; } - _exit(0); + _exit(EXIT_SUCCESS); } } @@ -703,13 +705,14 @@ virExec(virCommandPtr cmd) else execv(binary, cmd->args); + ret = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE; virReportSystemError(errno, _("cannot execute binary %s"), cmd->args[0]); fork_error: virDispatchError(NULL); - _exit(EXIT_FAILURE); + _exit(ret); cleanup: /* This is cleanup of parent process only - child diff --git a/tests/commandtest.c b/tests/commandtest.c index 2ae8871..042f049 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1,7 +1,7 @@ /* * commandtest.c: Test the libCommand API * - * 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 @@ -140,7 +140,7 @@ static int test1(const void *unused ATTRIBUTE_UNUSED) cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); if (virCommandRun(cmd, &status) < 0) goto cleanup; - if (status == 0) + if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_ENOENT) goto cleanup; ret = 0; @@ -899,6 +899,44 @@ cleanup: return ret; } +static int +test22(const void *unused ATTRIBUTE_UNUSED) +{ + int ret = -1; + virCommandPtr cmd; + int status = -1; + + cmd = virCommandNewArgList("/bin/sh", "-c", "exit 3", NULL); + + if (virCommandRun(cmd, &status) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + if (!WIFEXITED(status) || WEXITSTATUS(status) != 3) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + virCommandFree(cmd); + cmd = virCommandNewArgList("/bin/sh", "-c", "kill -9 $$", NULL); + + if (virCommandRun(cmd, &status) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } + if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; +} + static void virCommandThreadWorker(void *opaque) { virCommandTestDataPtr test = opaque; @@ -1046,6 +1084,7 @@ mymain(void) DO_TEST(test19); DO_TEST(test20); DO_TEST(test21); + DO_TEST(test22); virMutexLock(&test->lock); if (test->running) { -- 1.8.5.3

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 better matches 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(a)redhat.com&gt; --- 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 68c4c14..bd406db 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 b96dbd4..abe3635 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, -- 1.8.5.3

Auditing all callers of virCommandRun and virCommandWait that passed a non-NULL pointer for exit status turned up some interesting observations. Many callers were merely passing a pointer to avoid the overall command dying, but without caring what the exit status was - but these callers would be better off treating a child death by signal as an abnormal exit. Other callers were actually acting on the status, but not all of them remembered to filter by WIFEXITED and convert with WEXITSTATUS; depending on the platform, this can result in a status being reported as 256 times too big. And among those that correctly parse the output, it gets rather verbose. Finally, there were the callers that explicitly checked that the status was 0, and gave their own message, but with fewer details than what virCommand gives for free. So the best idea is to move the complexity out of callers and into virCommand - by default, we return the actual exit status already cleaned through WEXITSTATUS and treat signals as a failed command; but the few callers that care can ask for raw status and act on it themselves. * src/util/vircommand.h (virCommandRawStatus): New prototype. * src/libvirt_private.syms (util/command.h): Export it. * docs/internals/command.html.in: Document it. * src/util/vircommand.c (virCommandRawStatus): New function. (virCommandWait): Adjust semantics. * tests/commandtest.c (test1): Test it. * daemon/remote.c (remoteDispatchAuthPolkit): Adjust callers. * src/access/viraccessdriverpolkit.c (virAccessDriverPolkitCheck): Likewise. * src/fdstream.c (virFDStreamCloseInt): Likewise. * src/lxc/lxc_process.c (virLXCProcessStart): Likewise. * src/qemu/qemu_command.c (qemuCreateInBridgePortWithHelper): Likewise. * src/xen/xen_driver.c (xenUnifiedXendProbe): Simplify. * tests/reconnect.c (mymain): Likewise. * tests/statstest.c (mymain): Likewise. * src/bhyve/bhyve_process.c (virBhyveProcessStart) (virBhyveProcessStop): Don't overwrite virCommand error. * src/libvirt.c (virConnectAuthGainPolkit): Likewise. * src/openvz/openvz_driver.c (openvzDomainGetBarrierLimit) (openvzDomainSetBarrierLimit): Likewise. * src/util/virebtables.c (virEbTablesOnceInit): Likewise. * src/util/viriptables.c (virIpTablesOnceInit): Likewise. * src/util/virnetdevveth.c (virNetDevVethCreate): Fix debug message. * src/qemu/qemu_capabilities.c (virQEMUCapsInitQMP): Add comment. * src/storage/storage_backend_iscsi.c (virStorageBackendISCSINodeUpdate): Likewise. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- daemon/remote.c | 7 +++---- docs/internals/command.html.in | 17 +++++++++++++---- src/access/viraccessdriverpolkit.c | 9 ++++----- src/bhyve/bhyve_process.c | 19 +++---------------- src/fdstream.c | 3 ++- src/libvirt.c | 4 +--- src/libvirt_private.syms | 1 + src/lxc/lxc_process.c | 11 +++++++++-- src/openvz/openvz_driver.c | 18 +++++------------- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_command.c | 3 +-- src/storage/storage_backend_iscsi.c | 7 ++----- src/util/vircommand.c | 38 +++++++++++++++++++++++++++++++++---- src/util/vircommand.h | 2 ++ src/util/virebtables.c | 5 ++--- src/util/viriptables.c | 7 +++---- src/util/virnetdevveth.c | 4 ++-- src/xen/xen_driver.c | 9 ++++----- tests/commandtest.c | 23 ++++++++++++++++++++++ tests/reconnect.c | 3 +-- tests/statstest.c | 3 +-- 21 files changed, 117 insertions(+), 77 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 932f65f..b48d456 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3129,10 +3129,9 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, authdismissed = (pkout && strstr(pkout, "dismissed=true")); if (status != 0) { - char *tmp = virProcessTranslateStatus(status); - VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d: %s"), - action, (long long) callerPid, callerUid, NULLSTR(tmp)); - VIR_FREE(tmp); + VIR_ERROR(_("Policy kit denied action %s from pid %lld, uid %d " + "with status %d"), + action, (long long) callerPid, callerUid, status); goto authdeny; } PROBE(RPC_SERVER_CLIENT_AUTH_ALLOW, diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index 0336e65..f5c28af 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -430,7 +430,7 @@ if (string) VIR_DEBUG("about to run %s", string); VIR_FREE(string); - if (virCommandRun(cmd) &lt; 0) + if (virCommandRun(cmd, NULL) &lt; 0) return -1; </pre> @@ -458,15 +458,24 @@ non-zero exit status can represent a success condition, it is possible to request the exit status and perform that check manually instead of letting <code>virCommandRun</code> - raise the error + raise the error. By default, the captured status is only + for a normal exit (death from a signal is treated as an error), + but a caller can use <code>virCommandRawStatus</code> to get + encoded status that includes any terminating signals. </p> <pre> int status; if (virCommandRun(cmd, &amp;status) &lt; 0) - return -1; + return -1; + if (status == 1) { + ...do stuff... + } - if (WEXITSTATUS(status) ...) { + virCommandRawStatus(cmd2); + if (virCommandRun(cmd2, &amp;status) &lt; 0) + return -1; + if (WIFEXITED(status) &amp;&amp; WEXITSTATUS(status) == 1) { ...do stuff... } </pre> diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c index c325739..d9ebc49 100644 --- a/src/access/viraccessdriverpolkit.c +++ b/src/access/viraccessdriverpolkit.c @@ -1,7 +1,7 @@ /* * viraccessdriverpolkit.c: polkited access control driver * - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012, 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 @@ -170,11 +170,10 @@ virAccessDriverPolkitCheck(virAccessManagerPtr manager ATTRIBUTE_UNUSED, ret = 0; /* Denied */ } else { ret = -1; /* Error */ - char *tmp = virProcessTranslateStatus(status); virAccessError(VIR_ERR_ACCESS_DENIED, - _("Policy kit denied action %s from %s: %s"), - actionid, process, NULLSTR(tmp)); - VIR_FREE(tmp); + _("Policy kit denied action %s from %s: " + "exit status %d"), + actionid, process, status); } goto cleanup; } diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 7717fec..ee85680 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -57,7 +57,7 @@ virBhyveProcessStart(virConnectPtr conn, virCommandPtr cmd = NULL; virCommandPtr load_cmd = NULL; bhyveConnPtr privconn = conn->privateData; - int ret = -1, status; + int ret = -1; if (virAsprintf(&logfile, "%s/%s.log", BHYVE_LOG_DIR, vm->def->name) < 0) @@ -114,15 +114,9 @@ virBhyveProcessStart(virConnectPtr conn, virStrerror(errno, ebuf, sizeof(ebuf))); VIR_DEBUG("Loading domain '%s'", vm->def->name); - if (virCommandRun(load_cmd, &status) < 0) + if (virCommandRun(load_cmd, NULL) < 0) goto cleanup; - if (status != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Guest failed to load: %d"), status); - goto cleanup; - } - /* Now we can start the domain */ VIR_DEBUG("Starting domain '%s'", vm->def->name); ret = virCommandRun(cmd, NULL); @@ -165,7 +159,6 @@ virBhyveProcessStop(bhyveConnPtr driver, { size_t i; int ret = -1; - int status; virCommandPtr cmd = NULL; if (!virDomainObjIsActive(vm)) { @@ -203,15 +196,9 @@ virBhyveProcessStop(bhyveConnPtr driver, if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm))) goto cleanup; - if (virCommandRun(cmd, &status) < 0) + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (status != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Guest failed to stop: %d"), status); - goto cleanup; - } - ret = 0; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); diff --git a/src/fdstream.c b/src/fdstream.c index f7dae39..04d62b8 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -1,7 +1,7 @@ /* * fdstream.c: generic streams impl for file descriptors * - * Copyright (C) 2009-2012 Red Hat, Inc. + * Copyright (C) 2009-2012, 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 @@ -302,6 +302,7 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort) else buf[len] = '\0'; + virCommandRawStatus(fdst->cmd); if (virCommandWait(fdst->cmd, &status) < 0) { ret = -1; } else if (status != 0) { diff --git a/src/libvirt.c b/src/libvirt.c index f38556a..f5d85db 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -145,15 +145,13 @@ static int virConnectAuthGainPolkit(const char *privilege) { virCommandPtr cmd; - int status; int ret = -1; if (geteuid() == 0) return 0; cmd = virCommandNewArgList(POLKIT_AUTH, "--obtain", privilege, NULL); - if (virCommandRun(cmd, &status) < 0 || - status > 0) + if (virCommandRun(cmd, NULL) < 0) goto cleanup; ret = 0; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d7a9ee7..9b27043 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1102,6 +1102,7 @@ virCommandNewArgList; virCommandNewArgs; virCommandNonblockingFDs; virCommandPassFD; +virCommandRawStatus; virCommandRequireHandshake; virCommandRun; virCommandRunAsync; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 8989245..ccd1efb 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1199,12 +1199,19 @@ int virLXCProcessStart(virConnectPtr conn, VIR_WARN("Unable to seek to end of logfile: %s", virStrerror(errno, ebuf, sizeof(ebuf))); + virCommandRawStatus(cmd); if (virCommandRun(cmd, &status) < 0) goto cleanup; if (status != 0) { - if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf, sizeof(ebuf)) <= 0) - snprintf(ebuf, sizeof(ebuf), "unexpected exit status %d", status); + if (virLXCProcessReadLogOutput(vm, logfile, pos, ebuf, + sizeof(ebuf)) <= 0) { + if (WIFEXITED(status)) + snprintf(ebuf, sizeof(ebuf), _("unexpected exit status %d"), + WEXITSTATUS(status)); + else + snprintf(ebuf, sizeof(ebuf), "%s", _("terminated abnormally")); + } virReportError(VIR_ERR_INTERNAL_ERROR, _("guest failed to start: %s"), ebuf); goto cleanup; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b27ac4c..393f397 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1,7 +1,7 @@ /* * openvz_driver.c: core driver methods for managing OpenVZ VEs * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * Copyright (C) 2007 Anoop Joe Cyriac @@ -1710,7 +1710,7 @@ openvzDomainGetBarrierLimit(virDomainPtr domain, unsigned long long *barrier, unsigned long long *limit) { - int status, ret = -1; + int ret = -1; char *endp, *output = NULL; const char *tmp; virCommandPtr cmd = virCommandNewArgList(VZLIST, "--no-header", NULL); @@ -1718,12 +1718,8 @@ openvzDomainGetBarrierLimit(virDomainPtr domain, virCommandSetOutputBuffer(cmd, &output); virCommandAddArgFormat(cmd, "-o%s.b,%s.l", param, param); virCommandAddArg(cmd, domain->name); - if (virCommandRun(cmd, &status) < 0 || status != 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to get %s for %s: %d"), param, domain->name, - status); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - } tmp = output; virSkipSpaces(&tmp); @@ -1754,7 +1750,7 @@ openvzDomainSetBarrierLimit(virDomainPtr domain, unsigned long long barrier, unsigned long long limit) { - int status, ret = -1; + int ret = -1; virCommandPtr cmd = virCommandNewArgList(VZCTL, "--quiet", "set", NULL); /* LONG_MAX indicates unlimited so reject larger values */ @@ -1769,12 +1765,8 @@ openvzDomainSetBarrierLimit(virDomainPtr domain, virCommandAddArgFormat(cmd, "--%s", param); virCommandAddArgFormat(cmd, "%llu:%llu", barrier, limit); virCommandAddArg(cmd, "--save"); - if (virCommandRun(cmd, &status) < 0 || status != 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Failed to set %s for %s: %d"), param, domain->name, - status); + if (virCommandRun(cmd, NULL) < 0) goto cleanup; - } ret = 0; cleanup: diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d618b3f..22a946d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2693,6 +2693,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, virCommandSetGID(cmd, runGid); virCommandSetUID(cmd, runUid); + /* Log, but otherwise ignore, non-zero status. */ if (virCommandRun(cmd, &status) < 0) goto cleanup; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9ee84a0..ff0b2d5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -232,7 +232,6 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg, unsigned int flags) { virCommandPtr cmd; - int status; int pair[2] = { -1, -1 }; if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP) @@ -269,7 +268,7 @@ static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg, } if (virNetDevTapGetName(*tapfd, ifname) < 0 || - virCommandWait(cmd, &status) < 0) { + virCommandWait(cmd, NULL) < 0) { VIR_FORCE_CLOSE(*tapfd); *tapfd = -1; } diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 556c2cc..5479663 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -1,7 +1,7 @@ /* * storage_backend_iscsi.c: storage backend for iSCSI handling * - * Copyright (C) 2007-2008, 2010-2012 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -111,10 +111,6 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); - /* Note that we ignore the exitstatus. Older versions of iscsiadm tools - * returned an exit status of > 0, even if they succeeded. We will just - * rely on whether session got filled in properly. - */ if (virStorageBackendRunProgRegex(pool, cmd, 1, @@ -681,6 +677,7 @@ virStorageBackendISCSINodeUpdate(const char *portal, "--value", value, NULL); + /* Ignore non-zero status. */ if (virCommandRun(cmd, &status) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to update '%s' of node mode for target '%s'"), diff --git a/src/util/vircommand.c b/src/util/vircommand.c index a4397b4..415b8c3 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -113,6 +113,7 @@ struct _virCommand { pid_t pid; char *pidfile; bool reap; + bool rawStatus; unsigned long long maxMemLock; unsigned int maxProcesses; @@ -1120,6 +1121,25 @@ virCommandNonblockingFDs(virCommandPtr cmd) cmd->flags |= VIR_EXEC_NONBLOCK; } +/** + * virCommandRawStatus: + * @cmd: the command to modify + * + * Mark this command as returning raw exit status via virCommandRun() or + * virCommandWait() (caller must use WIFEXITED() and friends, and can + * detect death from signals) instead of the default of only allowing + * normal exit status (caller must not use WEXITSTATUS(), and death from + * signals returns -1). + */ +void +virCommandRawStatus(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->rawStatus = true; +} + /* Add an environment variable to the cmd->env list. 'env' is a * string like "name=value". If the named environment variable is * already set, then it is replaced in the list. @@ -2045,7 +2065,11 @@ int virCommandExec(virCommandPtr cmd ATTRIBUTE_UNUSED) * Returns -1 on any error executing the * command. Returns 0 if the command executed, * with the exit status set. If @exitstatus is NULL, then the - * child must exit with status 0 for this to succeed. + * child must exit with status 0 for this to succeed. By default, + * a non-NULL @exitstatus contains the normal exit status of the child + * (death from a signal is treated as execution error); but if + * virCommandRawStatus() was used, it instead contains the raw exit + * status that the caller must then decipher using WIFEXITED() and friends. */ int virCommandRun(virCommandPtr cmd, int *exitstatus) @@ -2335,7 +2359,11 @@ cleanup: * 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. + * child must exit with status 0 for this to succeed. By default, + * a non-NULL @exitstatus contains the normal exit status of the child + * (death from a signal is treated as execution error); but if + * virCommandRawStatus() was used, it instead contains the raw exit + * status that the caller must then decipher using WIFEXITED() and friends. */ int virCommandWait(virCommandPtr cmd, int *exitstatus) @@ -2372,7 +2400,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, true); + ret = virProcessWait(cmd->pid, &status, true); if (cmd->flags & VIR_EXEC_ASYNC_IO) { cmd->flags &= ~VIR_EXEC_ASYNC_IO; virThreadJoin(cmd->asyncioThread); @@ -2390,7 +2418,9 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) if (ret == 0) { cmd->pid = -1; cmd->reap = false; - if (status) { + if (exitstatus && (cmd->rawStatus || WIFEXITED(status))) { + *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status); + } else if (status) { char *str = virCommandToString(cmd); char *st = virProcessTranslateStatus(status); bool haveErrMsg = cmd->errbuf && *cmd->errbuf && (*cmd->errbuf)[0]; diff --git a/src/util/vircommand.h b/src/util/vircommand.h index a743200..ec6185d 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -86,6 +86,8 @@ void virCommandDaemonize(virCommandPtr cmd); void virCommandNonblockingFDs(virCommandPtr cmd); +void virCommandRawStatus(virCommandPtr cmd); + void virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); diff --git a/src/util/virebtables.c b/src/util/virebtables.c index 67f281c..ce5e813 100644 --- a/src/util/virebtables.c +++ b/src/util/virebtables.c @@ -1,7 +1,7 @@ /* * virebtables.c: Helper APIs for managing ebtables * - * Copyright (C) 2007-2013 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * Copyright (C) 2009 IBM Corp. * * This library is free software; you can redistribute it and/or @@ -66,10 +66,9 @@ virEbTablesOnceInit(void) "firewalld support disabled for ebtables."); } else { virCommandPtr cmd = virCommandNew(firewall_cmd_path); - int status; virCommandAddArgList(cmd, "--state", NULL); - if (virCommandRun(cmd, &status) < 0 || status != 0) { + if (virCommandRun(cmd, NULL) < 0) { VIR_INFO("firewall-cmd found but disabled for ebtables"); VIR_FREE(firewall_cmd_path); firewall_cmd_path = NULL; diff --git a/src/util/viriptables.c b/src/util/viriptables.c index 9b78d86..9e03cc4 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -1,7 +1,7 @@ /* * viriptables.c: helper APIs for managing iptables * - * Copyright (C) 2007-2013 Red Hat, Inc. + * Copyright (C) 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 @@ -60,7 +60,6 @@ static int virIpTablesOnceInit(void) { virCommandPtr cmd; - int status; #if HAVE_FIREWALLD firewall_cmd_path = virFindFileInPath("firewall-cmd"); @@ -71,7 +70,7 @@ virIpTablesOnceInit(void) cmd = virCommandNew(firewall_cmd_path); virCommandAddArgList(cmd, "--state", NULL); - if (virCommandRun(cmd, &status) < 0 || status != 0) { + if (virCommandRun(cmd, NULL) < 0) { VIR_INFO("firewall-cmd found but disabled for iptables"); VIR_FREE(firewall_cmd_path); firewall_cmd_path = NULL; @@ -88,7 +87,7 @@ virIpTablesOnceInit(void) cmd = virCommandNew(IPTABLES_PATH); virCommandAddArgList(cmd, "-w", "-L", "-n", NULL); - if (virCommandRun(cmd, &status) < 0 || status != 0) { + if (virCommandRun(cmd, NULL) < 0) { VIR_INFO("xtables locking not supported by your iptables"); } else { VIR_INFO("using xtables locking for iptables"); diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index 25eb282..18cc1eb 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright IBM Corp. 2008 * * This library is free software; you can redistribute it and/or @@ -166,7 +166,7 @@ int virNetDevVethCreate(char** veth1, char** veth2) VIR_DEBUG("Failed to create veth host: %s guest: %s: %d", *veth1 ? *veth1 : veth1auto, - *veth1 ? *veth1 : veth1auto, + *veth2 ? *veth2 : veth2auto, status); VIR_FREE(veth1auto); VIR_FREE(veth2auto); diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 7506e8c..ee05fb4 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -308,16 +308,15 @@ xenUnifiedProbe(void) } #ifdef WITH_LIBXL -static int +static bool xenUnifiedXendProbe(void) { virCommandPtr cmd; - int status; - int ret = 0; + bool ret = false; cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL); - if (virCommandRun(cmd, &status) == 0 && status == 0) - ret = 1; + if (virCommandRun(cmd, NULL) == 0) + ret = true; virCommandFree(cmd); return ret; diff --git a/tests/commandtest.c b/tests/commandtest.c index c0391a5..b329bfb 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -141,6 +141,12 @@ static int test1(const void *unused ATTRIBUTE_UNUSED) cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); if (virCommandRun(cmd, &status) < 0) goto cleanup; + if (status != EXIT_ENOENT) + goto cleanup; + + virCommandRawStatus(cmd); + if (virCommandRun(cmd, &status) < 0) + goto cleanup; if (!WIFEXITED(status) || WEXITSTATUS(status) != EXIT_ENOENT) goto cleanup; ret = 0; @@ -914,6 +920,17 @@ test22(const void *unused ATTRIBUTE_UNUSED) printf("Cannot run child %s\n", err->message); goto cleanup; } + if (status != 3) { + printf("Unexpected status %d\n", status); + goto cleanup; + } + + virCommandRawStatus(cmd); + if (virCommandRun(cmd, &status) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + goto cleanup; + } if (!WIFEXITED(status) || WEXITSTATUS(status) != 3) { printf("Unexpected status %d\n", status); goto cleanup; @@ -922,6 +939,12 @@ test22(const void *unused ATTRIBUTE_UNUSED) virCommandFree(cmd); cmd = virCommandNewArgList("/bin/sh", "-c", "kill -9 $$", NULL); + if (virCommandRun(cmd, &status) == 0) { + printf("Death by signal not detected, status %d\n", status); + goto cleanup; + } + + virCommandRawStatus(cmd); if (virCommandRun(cmd, &status) < 0) { virErrorPtr err = virGetLastError(); printf("Cannot run child %s\n", err->message); diff --git a/tests/reconnect.c b/tests/reconnect.c index 09deb5d..f0779ad 100644 --- a/tests/reconnect.c +++ b/tests/reconnect.c @@ -15,7 +15,6 @@ mymain(void) bool ro = false; virConnectPtr conn; virDomainPtr dom; - int status; virCommandPtr cmd; struct utsname ut; @@ -26,7 +25,7 @@ mymain(void) if (strstr(ut.release, "xen") == NULL) return EXIT_AM_SKIP; cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL); - if (virCommandRun(cmd, &status) != 0 || status != 0) { + if (virCommandRun(cmd, NULL) < 0) { virCommandFree(cmd); return EXIT_AM_SKIP; } diff --git a/tests/statstest.c b/tests/statstest.c index 441cedb..7af152a 100644 --- a/tests/statstest.c +++ b/tests/statstest.c @@ -40,7 +40,6 @@ static int mymain(void) { int ret = 0; - int status; virCommandPtr cmd; struct utsname ut; @@ -51,7 +50,7 @@ mymain(void) if (strstr(ut.release, "xen") == NULL) return EXIT_AM_SKIP; cmd = virCommandNewArgList("/usr/sbin/xend", "status", NULL); - if (virCommandRun(cmd, &status) != 0 || status != 0) { + if (virCommandRun(cmd, NULL) < 0) { virCommandFree(cmd); return EXIT_AM_SKIP; } -- 1.8.5.3

Note that 'virsh lxc-enter-namespace' must double-fork, for two reasons: some namespaces can only be done from a single thread, while virsh is multithreaded; and because virsh can be run in batch mode where we must not corrupt the namespace of that execution upon return from the subsidiary command. When virt-login-shell was first written, it blindly copied from 'virsh lxc-enter-namespace', including the double-fork. But neither of the reasons for double forking apply to virt-login-shell (we are single-threaded, and we have nothing to do after the child completes that would require us to preserve a namespace), so we can simplify life by using a single fork. In turn, this will make it easier for a future patch to pass the child's exit status on to the invoking shell. In flattening to a single fork, note that closing the fds must be done after fork, because the parent process still needs to use fds to control the virConnectPtr; meanwhile, chdir can be done prior to forking (in fact, it's easier to report errors on anything attempted before forking). * tools/virt-login-shell.c (main): Single rather than double fork. (virLoginShellFini): Delete, by inlining actions instead. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virt-login-shell.c | 116 ++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 73 deletions(-) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index 666facf..75a5223 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -41,24 +41,8 @@ #include "vircommand.h" #define VIR_FROM_THIS VIR_FROM_NONE -static ssize_t nfdlist; -static int *fdlist; static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf"; -static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) -{ - size_t i; - - for (i = 0; i < nfdlist; i++) - VIR_FORCE_CLOSE(fdlist[i]); - VIR_FREE(fdlist); - nfdlist = 0; - if (dom) - virDomainFree(dom); - if (conn) - virConnectClose(conn); -} - static int virLoginShellAllowedUser(virConfPtr conf, const char *name, gid_t *groups) @@ -187,7 +171,6 @@ main(int argc, char **argv) pid_t cpid; int ret = EXIT_FAILURE; int status; - int status2; uid_t uid = getuid(); gid_t gid = getgid(); char *name = NULL; @@ -201,6 +184,10 @@ main(int argc, char **argv) int longindex = -1; int ngroups; gid_t *groups = NULL; + ssize_t nfdlist = 0; + int *fdlist = NULL; + int openmax; + size_t i; struct option opt[] = { {"help", no_argument, NULL, 'h'}, @@ -298,6 +285,13 @@ main(int argc, char **argv) } } + openmax = sysconf(_SC_OPEN_MAX); + if (openmax < 0) { + virReportSystemError(errno, "%s", + _("sysconf(_SC_OPEN_MAX) failed")); + goto cleanup; + } + if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0) goto cleanup; if (VIR_ALLOC(secmodel) < 0) @@ -308,76 +302,52 @@ main(int argc, char **argv) goto cleanup; if (virDomainGetSecurityLabel(dom, seclabel) < 0) goto cleanup; + if (virSetUIDGID(0, 0, NULL, 0) < 0) + goto cleanup; + if (virDomainLxcEnterSecurityLabel(secmodel, seclabel, NULL, 0) < 0) + goto cleanup; + if (nfdlist > 0 && + virDomainLxcEnterNamespace(dom, nfdlist, fdlist, NULL, NULL, 0) < 0) + goto cleanup; + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) + goto cleanup; + if (chdir(homedir) < 0) { + virReportSystemError(errno, _("Unable to chdir(%s)"), homedir); + goto cleanup; + } - /* Fork once because we don't want to affect virt-login-shell's - * namespace itself. FIXME: is this necessary? - */ + /* A fork is required to create new process in correct pid namespace. */ if ((cpid = virFork()) < 0) goto cleanup; if (cpid == 0) { - pid_t ccpid; - - int openmax = sysconf(_SC_OPEN_MAX); - int fd; + int tmpfd; - if (virSetUIDGID(0, 0, NULL, 0) < 0) - return EXIT_FAILURE; - - if (virDomainLxcEnterSecurityLabel(secmodel, - seclabel, - NULL, - 0) < 0) - return EXIT_FAILURE; - - if (nfdlist > 0) { - if (virDomainLxcEnterNamespace(dom, - nfdlist, - fdlist, - NULL, - NULL, - 0) < 0) - return EXIT_FAILURE; - } - - ret = virSetUIDGID(uid, gid, groups, ngroups); - VIR_FREE(groups); - if (ret < 0) - return EXIT_FAILURE; - - if (openmax < 0) { - virReportSystemError(errno, "%s", - _("sysconf(_SC_OPEN_MAX) failed")); - return EXIT_FAILURE; - } - for (fd = 3; fd < openmax; fd++) { - int tmpfd = fd; + for (i = 3; i < openmax; i++) { + tmpfd = i; VIR_MASS_CLOSE(tmpfd); } - - /* A fork is required to create new process in correct pid - * namespace. */ - if ((ccpid = virFork()) < 0) + if (execv(shargv[0], (char *const*) shargv) < 0) { + virReportSystemError(errno, _("Unable to exec shell %s"), + shargv[0]); return EXIT_FAILURE; - - if (ccpid == 0) { - if (chdir(homedir) < 0) { - virReportSystemError(errno, _("Unable to chdir(%s)"), homedir); - return EXIT_FAILURE; - } - if (execv(shargv[0], (char *const*) shargv) < 0) { - virReportSystemError(errno, _("Unable to exec shell %s"), - shargv[0]); - return EXIT_FAILURE; - } } - return virProcessWait(ccpid, &status2, true); + return EXIT_SUCCESS; } - ret = virProcessWait(cpid, &status, true); + + if (virProcessWait(cpid, &status, true) < 0) + goto cleanup; + ret = EXIT_SUCCESS; cleanup: + for (i = 0; i < nfdlist; i++) + VIR_FORCE_CLOSE(fdlist[i]); + VIR_FREE(fdlist); virConfFree(conf); - virLoginShellFini(conn, dom); + if (dom) + virDomainFree(dom); + if (conn) + virConnectClose(conn); virStringFreeList(shargv); VIR_FREE(name); VIR_FREE(homedir); -- 1.8.5.3

On 03/05/2014 06:57 AM, John Ferlan wrote: Yep. Fixing with this, as the obvious followup: diff --git i/tools/virt-login-shell.c w/tools/virt-login-shell.c index 3ea7ade..abe7eeb 100644 --- i/tools/virt-login-shell.c +++ w/tools/virt-login-shell.c @@ -339,8 +339,9 @@ main(int argc, char **argv) /* At this point, the parent is now waiting for the child to exit, * but as that may take a long time, we release resources now. */ cleanup: - for (i = 0; i < nfdlist; i++) - VIR_FORCE_CLOSE(fdlist[i]); + if (nfdlist > 0) + for (i = 0; i < nfdlist; i++) + VIR_FORCE_CLOSE(fdlist[i]); VIR_FREE(fdlist); virConfFree(conf); if (dom) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

virt-login-shell was exiting with status 0, regardless of what the wrapped shell returned. This is unkind to users; we should behave more like env(1), nice(1), su(1), and other wrapper programs, by preserving the invoked application's status (which includes the distinction between death due to signal vs. normal death). * tools/virt-login-shell.c (main): Pass through child exit status. * tools/virt-login-shell.pod: Document exit status. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virt-login-shell.c | 35 +++++++++++++++++++---------------- tools/virt-login-shell.pod | 25 +++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index 75a5223..3ea7ade 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -21,13 +21,14 @@ */ #include <config.h> -#include <stdarg.h> -#include <getopt.h> -#include <stdio.h> #include <errno.h> -#include <stdlib.h> #include <fnmatch.h> +#include <getopt.h> #include <locale.h> +#include <signal.h> +#include <stdarg.h> +#include <stdio.h> +#include <stdlib.h> #include "internal.h" #include "virerror.h" @@ -168,8 +169,8 @@ main(int argc, char **argv) { virConfPtr conf = NULL; const char *login_shell_path = conf_file; - pid_t cpid; - int ret = EXIT_FAILURE; + pid_t cpid = -1; + int ret = EXIT_CANCELED; int status; uid_t uid = getuid(); gid_t gid = getgid(); @@ -195,8 +196,8 @@ main(int argc, char **argv) {NULL, 0, NULL, 0} }; if (virInitialize() < 0) { - fprintf(stderr, _("Failed to initialize libvirt Error Handling")); - return EXIT_FAILURE; + fprintf(stderr, _("Failed to initialize libvirt error handling")); + return EXIT_CANCELED; } setenv("PATH", "/bin:/usr/bin", 1); @@ -231,7 +232,7 @@ main(int argc, char **argv) case '?': default: usage(); - exit(EXIT_FAILURE); + exit(EXIT_CANCELED); } } @@ -330,15 +331,13 @@ main(int argc, char **argv) if (execv(shargv[0], (char *const*) shargv) < 0) { virReportSystemError(errno, _("Unable to exec shell %s"), shargv[0]); - return EXIT_FAILURE; + virDispatchError(NULL); + return errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE; } - return EXIT_SUCCESS; } - if (virProcessWait(cpid, &status, true) < 0) - goto cleanup; - ret = EXIT_SUCCESS; - + /* At this point, the parent is now waiting for the child to exit, + * but as that may take a long time, we release resources now. */ cleanup: for (i = 0; i < nfdlist; i++) VIR_FORCE_CLOSE(fdlist[i]); @@ -354,7 +353,11 @@ cleanup: VIR_FREE(seclabel); VIR_FREE(secmodel); VIR_FREE(groups); - if (ret) + + if (virProcessWait(cpid, &status, true) == 0) + virProcessExitWithStatus(status); + + if (virGetLastError()) virDispatchError(NULL); return ret; } diff --git a/tools/virt-login-shell.pod b/tools/virt-login-shell.pod index bcd7855..56861f7 100644 --- a/tools/virt-login-shell.pod +++ b/tools/virt-login-shell.pod @@ -4,7 +4,7 @@ virt-login-shell - tool to execute a shell within a container matching the users =head1 SYNOPSIS -B<virt-login-shell> +B<virt-login-shell> [I<OPTION>] =head1 DESCRIPTION @@ -47,6 +47,27 @@ variable in /etc/libvirt/virt-login-shell.conf. eg. allowed_users = [ "tom", "dick", "harry" ] +=head1 EXIT STATUS + +B<virt-login-shell> normally returns the exit status of the command it +executed. If the command was killed by a signal, but that signal is not +fatal to virt-login-shell, then it returns the signal number plus 128. + +Exit status generated by B<virt-login-shell> itself: + +=over 4 + +=item B<0> An option was used to learn more about this binary. + +=item B<125> Generic error before attempting execution of the configured +shell; for example, if libvirtd is not running. + +=item B<126> The configured shell exists but could not be executed. + +=item B<127> The configured shell could not be found. + +=back + =head1 BUGS Report any bugs discovered to the libvirt community via the mailing @@ -61,7 +82,7 @@ Alternatively report bugs to your software distributor / vendor. =head1 COPYRIGHT -Copyright (C) 2013 Red Hat, Inc., and the authors listed in the +Copyright (C) 2013-2014 Red Hat, Inc., and the authors listed in the libvirt AUTHORS file. =head1 LICENSE -- 1.8.5.3