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

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(a)redhat.com&gt; > > 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.