[libvirt] [PATCH 17/26] Convert nwfilter ebiptablesTearOldRules to virFirewall

[Stefan Berger]

On 04/08/2014 11:38 AM, Daniel P. Berrange wrote:
> Convert the nwfilter ebiptablesTearOldRules method to use the
> virFirewall object APIs instead of creating shell scripts
> using virBuffer APIs. This provides a performance improvement
> through allowing direct use of firewalld dbus APIs and will
> facilitate automated testing.
>
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
>


>   static void
> @@ -4248,46 +4271,31 @@ ebiptablesTearNewRules(const char *ifname)
>   static int
>   ebiptablesTearOldRules(const char *ifname)
>   {
> -    virBuffer buf = VIR_BUFFER_INITIALIZER;
> -
> -    /* switch to new iptables user defined chains */
> -    if (iptables_cmd_path) {
> -        NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
> -
> -        iptablesUnlinkRootChains(&buf, ifname);
> -        iptablesRemoveRootChains(&buf, ifname);
> -
> -        iptablesRenameTmpRootChains(&buf, ifname);
> -        ebiptablesExecCLI(&buf, true, NULL);
> -    }
> -
> -    if (ip6tables_cmd_path) {
> -        NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);
> -
> -        iptablesUnlinkRootChains(&buf, ifname);
> -        iptablesRemoveRootChains(&buf, ifname);
> -
> -        iptablesRenameTmpRootChains(&buf, ifname);
> -        ebiptablesExecCLI(&buf, true, NULL);
> -    }
> -
> -    if (ebtables_cmd_path) {
> -        NWFILTER_SET_EBTABLES_SHELLVAR(&buf);
> -
> -        ebtablesUnlinkRootChain(&buf, true, ifname);
> -        ebtablesUnlinkRootChain(&buf, false, ifname);
> +    virFirewallPtr fw = virFirewallNew();
> +    int ret = -1;
>
> -        ebtablesRemoveSubChains(&buf, ifname);
> +    virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
>
> -        ebtablesRemoveRootChain(&buf, true, ifname);
> -        ebtablesRemoveRootChain(&buf, false, ifname);
> +    iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
> +    iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
> +    iptablesRenameTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
>
> -        ebtablesRenameTmpSubAndRootChains(&buf, ifname);
> +    iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
> +    iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
> +    iptablesRenameTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
>
> -        ebiptablesExecCLI(&buf, true, NULL);
> -    }
> +    ebtablesUnlinkRootChainFW(fw, true, ifname);
> +    ebtablesUnlinkRootChainFW(fw, false, ifname);
> +    ebtablesRemoveSubChainsFW(fw, ifname);
> +    ebtablesRemoveRootChainFW(fw, true, ifname);
> +    ebtablesRemoveRootChainFW(fw, false, ifname);
> +    ebtablesRenameTmpSubAndRootChainsFW(fw, ifname);
>
> -    return 0;
> +    virMutexLock(&execCLIMutex);
> +    ret = virFirewallApply(fw);
> +    virMutexUnlock(&execCLIMutex);
> +    virFirewallFree(fw);
> +    return ret;
>   }

Looks like the transformations I have seen in the other patches - really 
amazing!. I suppose we wouldn't get here if either iptables, ip6tables, 
or ebtables weren't installed?
Besides I see the lock being grabbed here. You shouldn't need this lock 
anymore if you lock in the virFirewall code, where I guess you have to 
have a libvirt-internal centralized lock (possibly 3 locks , one for 
iptables, ip6tables, and ebtables if they don't mutually influence each 
other -- would need to test this -- or one lock for all of them) in case 
of direct eb/ip/ip6tables execution and none in case of firewalld, which 
should do its own locking.

Regards,
Stefan