[libvirt] [PATCHv2 1/10] nwfilter: make ignoring non-zero status easier to follow

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