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

[Stefan Berger]

On 04/16/2014 07:55 AM, Daniel P. Berrange wrote:

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 <berra...@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.

With this explanation: ACK

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list