Convert nwfilter ebiptablesTearOldRules to virFirewall

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>

src/nwfilter/nwfilter_ebiptables_driver.c

@@ -694,16 +694,6 @@ _iptablesRemoveRootChainFW(virFirewallPtr fw,
 }
 
 
-static void
-iptablesRemoveRootChain(virBufferPtr buf,
-                        char prefix,
-                        bool incoming,
-                        const char *ifname)
-{
-    _iptablesRemoveRootChain(buf, prefix, incoming, ifname, false);
-}
-
-
 static void
 iptablesRemoveRootChainFW(virFirewallPtr fw,
                           virFirewallLayer layer,
@@ -736,16 +726,6 @@ iptablesRemoveTmpRootChains(virBufferPtr buf,
 }
 
 
-static void
-iptablesRemoveRootChains(virBufferPtr buf,
-                         const char *ifname)
-{
-    iptablesRemoveRootChain(buf, 'F', false, ifname);
-    iptablesRemoveRootChain(buf, 'F', true, ifname);
-    iptablesRemoveRootChain(buf, 'H', true, ifname);
-}
-
-
 static void
 iptablesRemoveRootChainsFW(virFirewallPtr fw,
                            virFirewallLayer layer,
@@ -924,16 +904,6 @@ _iptablesUnlinkRootChainFW(virFirewallPtr fw,
 }
 
 
-static void
-iptablesUnlinkRootChain(virBufferPtr buf,
-                        const char *basechain,
-                        char prefix,
-                        bool incoming, const char *ifname)
-{
-    _iptablesUnlinkRootChain(buf,
-                             basechain, prefix, incoming, ifname, false);
-}
-
 static void
 iptablesUnlinkRootChainFW(virFirewallPtr fw,
                           virFirewallLayer layer,
@@ -957,16 +927,6 @@ iptablesUnlinkTmpRootChain(virBufferPtr buf,
 }
 
 
-static void
-iptablesUnlinkRootChains(virBufferPtr buf,
-                         const char *ifname)
-{
-    iptablesUnlinkRootChain(buf, VIRT_OUT_CHAIN, 'F', false, ifname);
-    iptablesUnlinkRootChain(buf, VIRT_IN_CHAIN,  'F', true, ifname);
-    iptablesUnlinkRootChain(buf, HOST_IN_CHAIN,  'H', true, ifname);
-}
-
-
 static void
 iptablesUnlinkRootChainsFW(virFirewallPtr fw,
                            virFirewallLayer layer,
@@ -989,10 +949,11 @@ iptablesUnlinkTmpRootChains(virBufferPtr buf,
 
 
 static void
-iptablesRenameTmpRootChain(virBufferPtr buf,
+iptablesRenameTmpRootChainFW(virFirewallPtr fw,
-                           char prefix,
+                             virFirewallLayer layer,
-                           bool incoming,
+                             char prefix,
-                           const char *ifname)
+                             bool incoming,
+                             const char *ifname)
 {
     char tmpchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
     char tmpChainPrefix[2] = {
@@ -1009,20 +970,19 @@ iptablesRenameTmpRootChain(virBufferPtr buf,
     PRINT_IPT_ROOT_CHAIN(tmpchain, tmpChainPrefix, ifname);
     PRINT_IPT_ROOT_CHAIN(chain, chainPrefix, ifname);
 
-    virBufferAsprintf(buf,
+    virFirewallAddRule(fw, layer,
-                      "$IPT -E %s %s" CMD_SEPARATOR,
+                       "-E", tmpchain, chain, NULL);
-                      tmpchain,
-                      chain);
 }
 
 
 static void
-iptablesRenameTmpRootChains(virBufferPtr buf,
+iptablesRenameTmpRootChainsFW(virFirewallPtr fw,
-                            const char *ifname)
+                              virFirewallLayer layer,
+                              const char *ifname)
 {
-    iptablesRenameTmpRootChain(buf, 'F', false, ifname);
+    iptablesRenameTmpRootChainFW(fw, layer, 'F', false, ifname);
-    iptablesRenameTmpRootChain(buf, 'F', true, ifname);
+    iptablesRenameTmpRootChainFW(fw, layer, 'F', true, ifname);
-    iptablesRenameTmpRootChain(buf, 'H', true, ifname);
+    iptablesRenameTmpRootChainFW(fw, layer, 'H', true, ifname);
 }
 
 
@@ -3282,6 +3242,30 @@ ebtablesRenameTmpSubChain(virBufferPtr buf,
                       tmpchain, chain);
 }
 
+static void
+ebtablesRenameTmpSubChainFW(virFirewallPtr fw,
+                            int incoming,
+                            const char *ifname,
+                            const char *protocol)
+{
+    char tmpchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
+    char tmpChainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP
+                                     : CHAINPREFIX_HOST_OUT_TEMP;
+    char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN
+                                  : CHAINPREFIX_HOST_OUT;
+
+    if (protocol) {
+        PRINT_CHAIN(tmpchain, tmpChainPrefix, ifname, protocol);
+        PRINT_CHAIN(chain, chainPrefix, ifname, protocol);
+    } else {
+        PRINT_ROOT_CHAIN(tmpchain, tmpChainPrefix, ifname);
+        PRINT_ROOT_CHAIN(chain, chainPrefix, ifname);
+    }
+
+    virFirewallAddRule(fw, VIR_FIREWALL_LAYER_ETHERNET,
+                       "-t", "nat", "-E", tmpchain, chain, NULL);
+}
+
 static void
 ebtablesRenameTmpRootChain(virBufferPtr buf,
                            bool incoming,
@@ -3291,38 +3275,79 @@ ebtablesRenameTmpRootChain(virBufferPtr buf,
 }
 
 static void
-ebtablesRenameTmpSubAndRootChains(virBufferPtr buf,
+ebtablesRenameTmpRootChainFW(virFirewallPtr fw,
-                                  const char *ifname)
+                             bool incoming,
+                             const char *ifname)
+{
+    ebtablesRenameTmpSubChainFW(fw, incoming, ifname, NULL);
+}
+
+
+static int
+ebtablesRenameTmpSubAndRootChainsQuery(virFirewallPtr fw,
+                                       const char *const *lines,
+                                       void *opaque ATTRIBUTE_UNUSED)
+{
+    size_t i;
+    char newchain[MAX_CHAINNAME_LENGTH];
+
+    for (i = 0; lines[i] != NULL; i++) {
+        VIR_DEBUG("Considering '%s'", lines[i]);
+        char *tmp = strstr(lines[i], "-j ");
+        if (!tmp)
+            continue;
+        tmp = tmp + 3;
+        if (tmp[0] != CHAINPREFIX_HOST_IN_TEMP &&
+            tmp[0] != CHAINPREFIX_HOST_OUT_TEMP)
+            continue;
+        if (tmp[1] != '-')
+            continue;
+
+        ignore_value(virStrcpyStatic(newchain, tmp));
+        if (newchain[0] == CHAINPREFIX_HOST_IN_TEMP)
+            newchain[0] = CHAINPREFIX_HOST_IN;
+        else
+            newchain[0] = CHAINPREFIX_HOST_OUT;
+        VIR_DEBUG("Renaming chain '%s' to '%s'", tmp, newchain);
+        virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
+                               false, ebtablesRenameTmpSubAndRootChainsQuery,
+                               NULL,
+                               "-t", "nat", "-L", tmp, NULL);
+        virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
+                               true, NULL, NULL,
+                               "-t", "nat", "-F", newchain, NULL);
+        virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
+                               true, NULL, NULL,
+                               "-t", "nat", "-X", newchain, NULL);
+        virFirewallAddRule(fw, VIR_FIREWALL_LAYER_ETHERNET,
+                           "-t", "nat", "-E", tmp, newchain, NULL);
+    }
+
+    return 0;
+}
+
+
+static void
+ebtablesRenameTmpSubAndRootChainsFW(virFirewallPtr fw,
+                                    const char *ifname)
 {
     char rootchain[MAX_CHAINNAME_LENGTH];
     size_t i;
     char chains[3] = {
         CHAINPREFIX_HOST_IN_TEMP,
         CHAINPREFIX_HOST_OUT_TEMP,
-        0};
+        0
-
+    };
-    NWFILTER_SET_EBTABLES_SHELLVAR(buf);
-
-    virBufferAsprintf(buf, NWFILTER_FUNC_COLLECT_CHAINS,
-                      chains);
-    virBufferAsprintf(buf, NWFILTER_FUNC_RENAME_CHAINS,
-                      CHAINPREFIX_HOST_IN_TEMP,
-                      CHAINPREFIX_HOST_IN,
-                      CHAINPREFIX_HOST_OUT_TEMP,
-                      CHAINPREFIX_HOST_OUT);
-
-    virBufferAsprintf(buf, NWFILTER_FUNC_SET_IFS);
-    virBufferAddLit(buf, "chains=\"$(collect_chains");
     for (i = 0; chains[i] != 0; i++) {
         PRINT_ROOT_CHAIN(rootchain, chains[i], ifname);
-        virBufferAsprintf(buf, " %s", rootchain);
+        virFirewallAddRuleFull(fw, VIR_FIREWALL_LAYER_ETHERNET,
+                               false, ebtablesRenameTmpSubAndRootChainsQuery,
+                               NULL,
+                               "-t", "nat", "-L", rootchain, NULL);
     }
-    virBufferAddLit(buf, ")\"\n");
 
-    virBufferAddLit(buf, "rename_chains $chains\n");
+    ebtablesRenameTmpRootChainFW(fw, true, ifname);
-
+    ebtablesRenameTmpRootChainFW(fw, false, ifname);
-    ebtablesRenameTmpRootChain(buf, true, ifname);
-    ebtablesRenameTmpRootChain(buf, false, ifname);
 }
 
 static void
@@ -4261,46 +4286,31 @@ ebiptablesTearNewRules(const char *ifname)
 static int
 ebiptablesTearOldRules(const char *ifname)
 {
-    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    virFirewallPtr fw = virFirewallNew();
+    int ret = -1;
 
-    /* switch to new iptables user defined chains */
+    virFirewallStartTransaction(fw, VIR_FIREWALL_TRANSACTION_IGNORE_ERRORS);
-    if (iptables_cmd_path) {
-        NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
 
-        iptablesUnlinkRootChains(&buf, ifname);
+    iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
-        iptablesRemoveRootChains(&buf, ifname);
+    iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
+    iptablesRenameTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
 
-        iptablesRenameTmpRootChains(&buf, ifname);
+    iptablesUnlinkRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
-        ebiptablesExecCLI(&buf, true, NULL);
+    iptablesRemoveRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
-    }
+    iptablesRenameTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV6, ifname);
 
-    if (ip6tables_cmd_path) {
+    ebtablesUnlinkRootChainFW(fw, true, ifname);
-        NWFILTER_SET_IP6TABLES_SHELLVAR(&buf);
+    ebtablesUnlinkRootChainFW(fw, false, ifname);
+    ebtablesRemoveSubChainsFW(fw, ifname);
+    ebtablesRemoveRootChainFW(fw, true, ifname);
+    ebtablesRemoveRootChainFW(fw, false, ifname);
+    ebtablesRenameTmpSubAndRootChainsFW(fw, ifname);
 
-        iptablesUnlinkRootChains(&buf, ifname);
+    virMutexLock(&execCLIMutex);
-        iptablesRemoveRootChains(&buf, ifname);
+    ret = virFirewallApply(fw);
-
+    virMutexUnlock(&execCLIMutex);
-        iptablesRenameTmpRootChains(&buf, ifname);
+    virFirewallFree(fw);
-        ebiptablesExecCLI(&buf, true, NULL);
+    return ret;
-    }
-
-    if (ebtables_cmd_path) {
-        NWFILTER_SET_EBTABLES_SHELLVAR(&buf);
-
-        ebtablesUnlinkRootChain(&buf, true, ifname);
-        ebtablesUnlinkRootChain(&buf, false, ifname);
-
-        ebtablesRemoveSubChains(&buf, ifname);
-
-        ebtablesRemoveRootChain(&buf, true, ifname);
-        ebtablesRemoveRootChain(&buf, false, ifname);
-
-        ebtablesRenameTmpSubAndRootChains(&buf, ifname);
-
-        ebiptablesExecCLI(&buf, true, NULL);
-    }
-
-    return 0;
 }
 
 

tests/nwfilterebiptablestest.c

@@ -96,6 +96,77 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque ATTRIBUTE_UNUSED)
 }
 
 
+static int
+testNWFilterEBIPTablesTearOldRules(const void *opaque ATTRIBUTE_UNUSED)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    const char *expected =
+        "iptables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FO-vnet0\n"
+        "iptables -D libvirt-out -m physdev --physdev-out vnet0 -g FO-vnet0\n"
+        "iptables -D libvirt-in -m physdev --physdev-in vnet0 -g FI-vnet0\n"
+        "iptables -D libvirt-host-in -m physdev --physdev-in vnet0 -g HI-vnet0\n"
+        "iptables -F FO-vnet0\n"
+        "iptables -X FO-vnet0\n"
+        "iptables -F FI-vnet0\n"
+        "iptables -X FI-vnet0\n"
+        "iptables -F HI-vnet0\n"
+        "iptables -X HI-vnet0\n"
+        "iptables -E FP-vnet0 FO-vnet0\n"
+        "iptables -E FJ-vnet0 FI-vnet0\n"
+        "iptables -E HJ-vnet0 HI-vnet0\n"
+        "ip6tables -D libvirt-out -m physdev --physdev-is-bridged --physdev-out vnet0 -g FO-vnet0\n"
+        "ip6tables -D libvirt-out -m physdev --physdev-out vnet0 -g FO-vnet0\n"
+        "ip6tables -D libvirt-in -m physdev --physdev-in vnet0 -g FI-vnet0\n"
+        "ip6tables -D libvirt-host-in -m physdev --physdev-in vnet0 -g HI-vnet0\n"
+        "ip6tables -F FO-vnet0\n"
+        "ip6tables -X FO-vnet0\n"
+        "ip6tables -F FI-vnet0\n"
+        "ip6tables -X FI-vnet0\n"
+        "ip6tables -F HI-vnet0\n"
+        "ip6tables -X HI-vnet0\n"
+        "ip6tables -E FP-vnet0 FO-vnet0\n"
+        "ip6tables -E FJ-vnet0 FI-vnet0\n"
+        "ip6tables -E HJ-vnet0 HI-vnet0\n"
+        "ebtables -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0\n"
+        "ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0\n"
+        "ebtables -t nat -L libvirt-I-vnet0\n"
+        "ebtables -t nat -L libvirt-O-vnet0\n"
+        "ebtables -t nat -F libvirt-I-vnet0\n"
+        "ebtables -t nat -X libvirt-I-vnet0\n"
+        "ebtables -t nat -F libvirt-O-vnet0\n"
+        "ebtables -t nat -X libvirt-O-vnet0\n"
+        "ebtables -t nat -L libvirt-J-vnet0\n"
+        "ebtables -t nat -L libvirt-P-vnet0\n"
+        "ebtables -t nat -E libvirt-J-vnet0 libvirt-I-vnet0\n"
+        "ebtables -t nat -E libvirt-P-vnet0 libvirt-O-vnet0\n";
+    char *actual = NULL;
+    int ret = -1;
+
+    virCommandSetDryRun(&buf, NULL, NULL);
+
+    if (ebiptables_driver.tearOldRules("vnet0") < 0)
+        goto cleanup;
+
+    if (virBufferError(&buf))
+        goto cleanup;
+
+    actual = virBufferContentAndReset(&buf);
+    virtTestClearCommandPath(actual);
+
+    if (STRNEQ_NULLABLE(actual, expected)) {
+        virtTestDifference(stderr, actual, expected);
+        goto cleanup;
+    }
+
+    ret = 0;
+ cleanup:
+    virCommandSetDryRun(NULL, NULL, NULL);
+    virBufferFreeAndReset(&buf);
+    VIR_FREE(actual);
+    return ret;
+}
+
+
 static int
 mymain(void)
 {
@@ -111,6 +182,11 @@ mymain(void)
                     NULL) < 0)
         ret = -1;
 
+    if (virtTestRun("ebiptablesTearOldRules",
+                    testNWFilterEBIPTablesTearOldRules,
+                    NULL) < 0)
+        ret = -1;
+
  cleanup:
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }