Re: [PATCH 23/26] Convert nwfilter ebiptablesApplyNewRules to virFirewall

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 04/08/2014 11:38 AM, Daniel P. Berrange wrote:
Convert the nwfilter ebtablesApplyNewRules 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@xxxxxxxxxx>
[...]
@@ -679,8 +540,12 @@ _iptablesRemoveRootChainFW(virFirewallPtr fw,

      PRINT_IPT_ROOT_CHAIN(chain, chainPrefix, ifname);

-    virFirewallAddRule(fw, layer, "-F", chain, NULL);
-    virFirewallAddRule(fw, layer, "-X", chain, NULL);
+    virFirewallAddRuleFull(fw, layer,
+                           true, NULL, NULL,
+                           "-F", chain, NULL);
+    virFirewallAddRuleFull(fw, layer,
+                           true, NULL, NULL,
+                           "-X", chain, NULL);
  }

Looks like I didn't spot this in a previous patch. We have to ignore the errors here... on -F, -X and -D probably everywhere.


@@ -1088,138 +866,116 @@ iptablesHandleIpHdr(virBufferPtr buf,
          dstrange = "--src-range";
      }

-    if (HAS_ENTRY_ITEM(&ipHdr->dataIPSet) &&
-        HAS_ENTRY_ITEM(&ipHdr->dataIPSetFlags)) {
-
-        if (printDataType(vars,
-                          str, sizeof(str),
-                          &ipHdr->dataIPSet) < 0)
-            goto err_exit;
-
-        virBufferAsprintf(afterStateMatch,
-                          " -m set --match-set \"%s\" ",
-                          str);
-
-        if (printDataTypeDirection(vars,
-                                   str, sizeof(str),
-                                   &ipHdr->dataIPSetFlags, directionIn) < 0)
-            goto err_exit;
-
-        virBufferAdd(afterStateMatch, str, -1);
-    }
-

You are removing this entirely? ... ah I see it further below... Oh, I see, you moved it because of the afterStateMatch buffer that this part is using.


      }

      if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) {
-        printCommentVar(prefix, ipHdr->dataComment.u.string);
-
          /* keep comments behind everything else -- they are packet eval.
             no-ops */
-        virBufferAddLit(afterStateMatch,
-                        " -m comment --comment \"$" COMMENT_VARNAME "\"");
+        virFirewallRuleAddArgList(fw, fwrule,
+                                  "-m", "comment",
+                                  "--comment", ipHdr->dataComment.u.string,
+                                  NULL);
      }

The interesting part about comments was before to ensure that $(foo) never executes in a subshell. With TCK passing it seems this concern is addressed.



@@ -4038,188 +3457,173 @@ ebiptablesApplyNewRules(const char *ifname,
          }
      }

-    /* process ebtables commands; interleave commands from filters with
-       commands for creating and connecting ebtables chains */
-    j = 0;
      for (i = 0; i < nrules; i++) {
          if (virNWFilterRuleIsProtocolEthernet(rules[i]->def)) {
-            while (j < nEbtChains &&
-                   ebtChains[j].priority <= rules[i]->priority) {
-                ebiptablesInstCommand(&buf,
-                                      ebtChains[j++].commandTemplate);
-            }
-            ebtablesRuleInstCommand(&buf,
-                                    ifname,
-                                    rules[i]);
+            haveEbtables = true;
          } else {
              if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
                  haveIptables = true;
-            else if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
+            else if (virNWFilterRuleIsProtocolIPv6(rules[i]->def))
                  haveIp6tables = true;

Ah, this is probably the reason why IPv6 rules didn't work in TCK and 23/26 now fixes it. That's probably a typo introduced in 8/26. If you want to go back to 8/26 to fix this ... unless this has other negative side effects during the surgery. Up to you.


          }
      }
+    /* process ebtables commands; interleave commands from filters with
+       commands for creating and connecting ebtables chains */
+    if (haveEbtables) {

-    while (j < nEbtChains)
-        ebiptablesInstCommand(&buf,
-                              ebtChains[j++].commandTemplate);
+        /* scan the rules to see which chains need to be created */
+        for (i = 0; i < nrules; i++) {
+            if (virNWFilterRuleIsProtocolEthernet(rules[i]->def)) {
+                const char *name = rules[i]->chainSuffix;
+                if (rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_OUT ||
+                    rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) {
+                    if (virHashUpdateEntry(chains_in_set, name,
+                                           &rules[i]->chainPriority) < 0)
+                        goto cleanup;
+                }
+                if (rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_IN ||
+                    rules[i]->def->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) {
+                    if (virHashUpdateEntry(chains_out_set, name,
+                                           &rules[i]->chainPriority) < 0)
+                        goto cleanup;
+                }
+            }
+        }

-    if (ebiptablesExecCLI(&buf, false, &errmsg) < 0)
-        goto tear_down_tmpebchains;
+        /* create needed chains */
+        if (virHashSize(chains_in_set) > 0) {
+            ebtablesCreateTmpRootChainFW(fw, true, ifname);
+            if (ebtablesGetSubChainInsts(chains_in_set,
+                                         true,
+                                         &subchains,
+                                         &nsubchains) < 0)
+                goto cleanup;
+        }
+        if (virHashSize(chains_out_set) > 0) {
+            ebtablesCreateTmpRootChainFW(fw, false, ifname);
+            if (ebtablesGetSubChainInsts(chains_out_set,
+                                         false,
+                                         &subchains,
+                                         &nsubchains) < 0)
+                goto cleanup;
+        }
+
+        if (nsubchains > 0)
+            qsort(subchains, nsubchains, sizeof(subchains[0]),
+                  ebtablesSubChainInstSort);
+
+        for (i = 0, j = 0; i < nrules; i++) {
+            if (virNWFilterRuleIsProtocolEthernet(rules[i]->def)) {
+                while (j < nsubchains &&
+                       subchains[j]->priority <= rules[i]->priority) {
+                    ebtablesCreateTmpSubChainFW(fw,
+                                                subchains[j]->incoming,
+                                                ifname,
+                                                subchains[j]->protoidx,
+                                                subchains[j]->filtername);
+                    j++;
+                }
+                if (ebtablesRuleInstCommand(fw,
+                                            ifname,
+                                            rules[i]) < 0)
+                    goto cleanup;
+            }
+        }
+        while (j < nsubchains) {
+            ebtablesCreateTmpSubChainFW(fw,
+                                        subchains[j]->incoming,
+                                        ifname,
+                                        subchains[j]->protoidx,
+                                        subchains[j]->filtername);
+            j++;
+        }
+    }

      if (haveIptables) {

Based on your comment in another patch, now I am surprised to still see this check 'haveIptables' here. Wouldn't the rpm also solve this here as well?

-        NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
-
-        iptablesUnlinkTmpRootChains(&buf, ifname);
-        iptablesRemoveTmpRootChains(&buf, ifname);
-
-        iptablesCreateBaseChains(&buf);
-
-        if (ebiptablesExecCLI(&buf, false, &errmsg) < 0)
-            goto tear_down_tmpebchains;
-
-        NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
+        iptablesUnlinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
+        iptablesRemoveTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);

-        iptablesCreateTmpRootChains(&buf, ifname);
+        iptablesCreateBaseChainsFW(fw, VIR_FIREWALL_LAYER_IPV4);
+        iptablesCreateTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);

-        if (ebiptablesExecCLI(&buf, false, &errmsg) < 0)
-           goto tear_down_tmpiptchains;
-
-        NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
-
-        iptablesLinkTmpRootChains(&buf, ifname);
-        iptablesSetupVirtInPost(&buf, ifname);
-        if (ebiptablesExecCLI(&buf, false, &errmsg) < 0)
-           goto tear_down_tmpiptchains;
-
-        NWFILTER_SET_IPTABLES_SHELLVAR(&buf);
+        iptablesLinkTmpRootChainsFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);
+        iptablesSetupVirtInPostFW(fw, VIR_FIREWALL_LAYER_IPV4, ifname);

          for (i = 0; i < nrules; i++) {
-            if (virNWFilterRuleIsProtocolIPv4(rules[i]->def))
-                iptablesRuleInstCommand(&buf,
-                                        ifname,
-                                        rules[i]);
+            if (virNWFilterRuleIsProtocolIPv4(rules[i]->def)) {
+                if (iptablesRuleInstCommand(fw,
+                                            ifname,
+                                            rules[i]) < 0)
+                    goto cleanup;
+            }
          }

-        if (ebiptablesExecCLI(&buf, false, &errmsg) < 0)
-           goto tear_down_tmpiptchains;
-
          iptablesCheckBridgeNFCallEnabled(false);
      }

      if (haveIp6tables) {

... also this here.

Tentative ACK

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]