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@xxxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list