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

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