On 02/20/2014 07:13 AM, Eric Blake wrote:
> 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.
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 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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list