[libvirt] [PATCH V5 09/10] Interleave jumping into chains with filtering rules in root table

[Stefan Berger]

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.