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

On Wed, Apr 16, 2014 at 07:41:10AM -0400, Stefan Berger wrote: > 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? The RPM will ensure they are all available, and the virfirewall code will complain if they're missing, so IMHO that's sufficient. > 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. These locks are just to protect things during the intermediate part-converted stage. They go away at the end of this series so we rely on the lock in virfirewall.c, which obsoletes the execCLIMutex.