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

[Eric Blake]

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,

>                        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.

> +    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.

>  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.

> @@ -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).

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111117/93afa36a/attachment-0001.sig>