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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]