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