Re: [libvirt] [PATCH 01/10] nwfilter: don't ignore child process failures

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(a)redhat.com&gt; --- 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;