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

2011-11-18 Thread 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,
-

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

2011-11-17 Thread Stefan Berger 
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.

v3:
 - fix memory leak

Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com

---
 src/nwfilter/nwfilter_ebiptables_driver.c |  115 ++
 1 file changed, 103 insertions(+), 12 deletions(-)

Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
===
--- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c
@@ -2718,13 +2718,17 @@ ebtablesUnlinkTmpRootChain(virBufferPtr 
 
 
 static int
-ebtablesCreateTmpSubChain(virBufferPtr buf,
+ebtablesCreateTmpSubChain(ebiptablesRuleInstPtr *inst,
+  int *nRuleInstances,
   int incoming,
   const char *ifname,
   enum l3_proto_idx protoidx,
   const char *filtername,
-  int stopOnError)
+  int stopOnError,
+  virNWFilterChainPriority priority)
 {
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+ebiptablesRuleInstPtr tmp = *inst;
 char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
 char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP
   : CHAINPREFIX_HOST_OUT_TEMP;
@@ -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
   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
   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]));
+tmp[*nRuleInstances].priority = priority;
+tmp[*nRuleInstances].commandTemplate =
+virBufferContentAndReset(buf);
+tmp[*nRuleInstances].neededProtocolChain =
+virNWFilterChainSuffixTypeToString(VIR_NWFILTER_CHAINSUFFIX_ROOT);
+
+(*nRuleInstances)++;
+
 return 0;
 }
 
@@ -3224,9 +3253,34 @@ static int ebtablesCleanAll(const char *
 static int
 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);
+}
+
+static int
+ebiptablesRuleOrderSortPtr(const void *a, const void *b)
+{
 const ebiptablesRuleInstPtr *insta = a;
 const

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

2011-11-17 Thread 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   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library