[libvirt] [PATCH V5 09/10] Interleave jumping into chains with filtering rules in root table
Stefan Berger
stefanb at linux.vnet.ibm.com
Fri Nov 18 12:00:53 UTC 2011
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.
More information about the libvir-list
mailing list