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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list