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

[Daniel P. Berrange]

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 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?

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 :|