On 11/17/2011 07:15 PM, Eric Blake wrote:
On 11/17/2011 01:11 PM, Stefan Berger wrote:
The previous patch extends the priority of filtering rules into negative
numbers. We now use this possibility to interleave the jumping into
chains with filtering rules to for example create the 'root' table of
an interface with the following sequence of rules:
Bridge chain: libvirt-I-vnet0, entries: 6, policy: ACCEPT
-p IPv4 -j I-vnet0-ipv4
-p ARP -j I-vnet0-arp
-p ARP -j ACCEPT
-p 0x8035 -j I-vnet0-rarp
-p 0x835 -j ACCEPT
-j DROP
The '-p ARP -j ACCEPT' rule now appears between the jumps.
Since the 'arp' chain has been assigned priority -700 and the 'rarp'
chain -600, the above ordering can now be achieved with the following
rule:
<rule action='accept' direction='out' priority='-650'>
<mac protocolid='arp'/>
</rule>
This patch now sorts the commands generating the above shown jumps into
chains and interleaves their execution with those for generating rules.
Overall, it looks like it does what you say. But it may be worth a v6.
@@ -2733,15 +2737,22 @@ ebtablesCreateTmpSubChain(virBufferPtr b
PRINT_CHAIN(chain, chainPrefix, ifname,
(filtername) ? filtername : l3_protocols[protoidx].val);
- virBufferAsprintf(buf,
+ virBufferAsprintf(&buf,
+ CMD_DEF("%s -t %s -F %s") CMD_SEPARATOR
+ CMD_EXEC
+ CMD_DEF("%s -t %s -X %s") CMD_SEPARATOR
+ CMD_EXEC
Looks like my comments on v4 4/10 would apply here as well - a patch
11/10 that refactored things to use a shell variable initialized once up
front, instead of passing repetitive command names through %s all over
the place, might make this generator easier to follow. But not a
problem for the context of this patch. This hunk adds 6 printf args,
I'll do it, but can we defer this patch for later so it doesn't cause
too much churn on all other pending patches (series)?
CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR
CMD_EXEC
"%s"
- CMD_DEF("%s -t %s -A %s -p 0x%x -j %s") CMD_SEPARATOR
+ CMD_DEF("%s -t %s -%%c %s %%s -p 0x%x -j %s")
+ CMD_SEPARATOR
and this hunk has no net effect, but generates a string which will be
handed as the format string to yet another printf? Wow, that's a bit
hard to follow...
CMD_EXEC
"%s",
ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain,
+ ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain,
+ ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain,
CMD_STOPONERR(stopOnError),
@@ -2750,6 +2761,24 @@ ebtablesCreateTmpSubChain(virBufferPtr b
CMD_STOPONERR(stopOnError));
+ if (virBufferError(&buf) ||
+ VIR_REALLOC_N(tmp, (*nRuleInstances)+1)< 0) {
+ virReportOOMError();
+ virBufferFreeAndReset(&buf);
+ return -1;
+ }
+
+ *inst = tmp;
+
+ memset(&tmp[*nRuleInstances], 0, sizeof(tmp[0]));
Using VIR_EXPAND_N instead of VIR_REALLOC_N would take care of this
memset for you.
With the side effect that I need an additional variable
count = *nRuleInstances;
Converted..
+ tmp[*nRuleInstances].priority = priority;
+ tmp[*nRuleInstances].commandTemplate =
+ virBufferContentAndReset(&buf);
...If I followed things correctly, commandTemplate now has exactly two
print arguments, %c and %s. But looking further, it looks like you
already have other commandTemplate uses just like this.
Yes, there are others. I had to convert it to be able to treat the
'jumping into subchains' equivalent to 'normal filtering rules'.
ebiptablesRuleOrderSort(const void *a, const void *b)
{
+ const ebiptablesRuleInstPtr insta = (const ebiptablesRuleInstPtr)a;
+ const ebiptablesRuleInstPtr instb = (const ebiptablesRuleInstPtr)b;
+ const char *root = virNWFilterChainSuffixTypeToString(
+ VIR_NWFILTER_CHAINSUFFIX_ROOT);
+ bool root_a = STREQ(insta->neededProtocolChain, root);
+ bool root_b = STREQ(instb->neededProtocolChain, root);
+
+ /* ensure root chain commands appear before all others since
+ we will need them to create the child chains */
+ if (root_a) {
+ if (root_b) {
+ goto normal;
+ }
+ return -1; /* a before b */
+ }
+ if (root_b) {
+ return 1; /* b before a */
+ }
+normal:
+ return (insta->priority - instb->priority);
Refer to my review earlier in the series about adding a comment how
priority is in a bounded range, so the subtraction is safe.
Done.
@@ -3315,8 +3372,11 @@ ebtablesCreateTmpRootAndSubChains(virBuf
filter_names[i].key);
if ((int)idx< 0)
continue;
- rc = ebtablesCreateTmpSubChain(buf, direction, ifname, idx,
- filter_names[i].key, 1);
+ priority = virHashLookup(chains, filter_names[i].key);
Why do a virHashLookup, when you already have filter_names[i].value?
(See, I knew there was a reason to return key/value pairs).
I guess I didn't pay enough attention when converting the code. Fixed
this instance.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list