[libvirt] [PATCH] Re-add use of locking with iptables/ip6tables/ebtables

[Serge Hallyn]

Quoting Daniel P. Berrange (berrange at redhat.com):
> A previous commit introduced use of locking with invocation
> of iptables in the viriptables.c module
> 
>   commit ba95426d6f39aec1da6e069dd7222f7a8c6a5862
>   Author: Serge Hallyn <serge.hallyn at ubuntu.com>
>   Date:   Fri Nov 1 12:36:59 2013 -0500
> 
>     util: use -w flag when calling iptables
> 
> This only ever had effect with the virtual network driver,
> as it was not wired up into the nwfilter driver. Unfortunately
> in the firewall refactoring the use of the -w flag was
> accidentally lost.
> 
> This patch introduces it to the virfirewall.c module so that
> both the virtual network and nwfilter drivers will be using
> it. It also ensures that the equivalent --concurrent flag
> to ebtables is used.
> ---

Thanks, that looks very nice.

Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>

>  src/util/virfirewall.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++---
>  src/util/viriptables.c |  2 --
>  2 files changed, 63 insertions(+), 6 deletions(-)
> 
> diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> index bab1634..c83fdc6 100644
> --- a/src/util/virfirewall.c
> +++ b/src/util/virfirewall.c
> @@ -104,6 +104,44 @@ virFirewallOnceInit(void)
>  
>  VIR_ONCE_GLOBAL_INIT(virFirewall)
>  
> +static bool iptablesUseLock;
> +static bool ip6tablesUseLock;
> +static bool ebtablesUseLock;
> +
> +static void
> +virFirewallCheckUpdateLock(bool *lockflag,
> +                           const char *const*args)
> +{
> +    virCommandPtr cmd = virCommandNewArgs(args);
> +    if (virCommandRun(cmd, NULL) < 0) {
> +        VIR_INFO("locking not supported by %s", args[0]);
> +    } else {
> +        VIR_INFO("using locking for %s", args[0]);
> +        *lockflag = true;
> +    }
> +    virCommandFree(cmd);
> +}
> +
> +static void
> +virFirewallCheckUpdateLocking(void)
> +{
> +    const char *iptablesArgs[] = {
> +        IPTABLES_PATH, "-w", "-L", "-n", NULL,
> +    };
> +    const char *ip6tablesArgs[] = {
> +        IP6TABLES_PATH, "-w", "-L", "-n", NULL,
> +    };
> +    const char *ebtablesArgs[] = {
> +        EBTABLES_PATH, "--concurrent", "-L", NULL,
> +    };
> +    virFirewallCheckUpdateLock(&iptablesUseLock,
> +                               iptablesArgs);
> +    virFirewallCheckUpdateLock(&ip6tablesUseLock,
> +                               ip6tablesArgs);
> +    virFirewallCheckUpdateLock(&ebtablesUseLock,
> +                               ebtablesArgs);
> +}
> +
>  static int
>  virFirewallValidateBackend(virFirewallBackend backend)
>  {
> @@ -161,6 +199,9 @@ virFirewallValidateBackend(virFirewallBackend backend)
>      }
>  
>      currentBackend = backend;
> +
> +    virFirewallCheckUpdateLocking();
> +
>      return 0;
>  }
>  
> @@ -201,6 +242,9 @@ virFirewallPtr virFirewallNew(void)
>  {
>      virFirewallPtr firewall;
>  
> +    if (virFirewallInitialize() < 0)
> +        return NULL;
> +
>      if (VIR_ALLOC(firewall) < 0)
>          return NULL;
>  
> @@ -321,6 +365,23 @@ virFirewallAddRuleFullV(virFirewallPtr firewall,
>      rule->queryOpaque = opaque;
>      rule->ignoreErrors = ignoreErrors;
>  
> +    switch (rule->layer) {
> +    case VIR_FIREWALL_LAYER_ETHERNET:
> +        if (ebtablesUseLock)
> +            ADD_ARG(rule, "--concurrent");
> +        break;
> +    case VIR_FIREWALL_LAYER_IPV4:
> +        if (iptablesUseLock)
> +            ADD_ARG(rule, "-w");
> +        break;
> +    case VIR_FIREWALL_LAYER_IPV6:
> +        if (ip6tablesUseLock)
> +            ADD_ARG(rule, "-w");
> +        break;
> +    case VIR_FIREWALL_LAYER_LAST:
> +        break;
> +    }
> +
>      while ((str = va_arg(args, char *)) != NULL) {
>          ADD_ARG(rule, str);
>      }
> @@ -840,8 +901,8 @@ virFirewallApplyGroup(virFirewallPtr firewall,
>      bool ignoreErrors = (group->actionFlags & VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
>      size_t i;
>  
> -    VIR_INFO("Starting transaction for %p flags=%x",
> -             group, group->actionFlags);
> +    VIR_INFO("Starting transaction for firewall=%p group=%p flags=%x",
> +             firewall, group, group->actionFlags);
>      firewall->currentGroup = idx;
>      group->addingRollback = false;
>      for (i = 0; i < group->naction; i++) {
> @@ -879,8 +940,6 @@ virFirewallApply(virFirewallPtr firewall)
>      int ret = -1;
>  
>      virMutexLock(&ruleLock);
> -    if (virFirewallInitialize() < 0)
> -        goto cleanup;
>  
>      if (!firewall || firewall->err == ENOMEM) {
>          virReportOOMError();
> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index 4f3ac9c..46b4017 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -52,8 +52,6 @@
>  
>  VIR_LOG_INIT("util.iptables");
>  
> -bool iptables_supports_xlock = false;
> -
>  #define VIR_FROM_THIS VIR_FROM_NONE
>  
>  enum {
> -- 
> 2.1.0
>