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@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?
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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|