Re: [libvirt] [PATCH V4 03/10] Make filter creation in root table more flexible

2011-11-17 Thread Stefan Berger 

On 11/16/2011 08:02 PM, Eric Blake wrote:

On 10/26/2011 05:36 AM, Stefan Berger wrote:

Use the previously introduced chain priorities to sort the chains for access
from an interface's 'root' table and have them created in the proper order.
This gets rid of a lot of code that was previously creating the chains in a
more hardcoded way.

To determine what protocol a filter is used for evaluation do prefix-
matching, i.e., the filter 'arp' is used to filter for the 'arp' protocol,
'ipv4' for the 'ipv4' protocol and 'arp-xyz' will also be used to filter
for the 'arp' protocol following the prefix 'arp' in its name.

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

---
  src/nwfilter/nwfilter_ebiptables_driver.c |  130 
++
  1 file changed, 98 insertions(+), 32 deletions(-)


+static int
+ebiptablesFilterOrderSort(const virHashKeyValuePairPtr a,
+  const virHashKeyValuePairPtr b)
+{
+return *(virNWFilterChainPriority *)a-value -
+   *(virNWFilterChainPriority *)b-value;
+}

I tend to worry about someone passing INT_MAX and INT_MIN to a qsort
algorithm, then getting the wrong comparison sense because of integer
overflow when it uses straight subtraction; so I like to add a comment
explaining why I know that the valid input was capped and overflow is
impossible.


Values here are limited to range [-1000, 1000]. Added that as a comment now.


  static void
  iptablesCheckBridgeNFCallEnabled(bool isIPv6)
@@ -3327,6 +3336,56 @@ iptablesCheckBridgeNFCallEnabled(bool is
  }
  }

+/*
+ * Given a filtername determine the protocol it is used for evaluating
+ * We do prefix-matching to determine the protocol.
+ */
+static enum l3_proto_idx
+ebtablesGetProtoIdxByFiltername(const char *filtername)
+{
+enum l3_proto_idx idx;
+
+for (idx = 0; idx  L3_PROTO_LAST_IDX; idx++) {
+if (STRPREFIX(filtername, l3_protocols[idx].val)) {
+return idx;
+}
+}

This works as long as none of the entries in l3_protocols are a prefix
of any other entry; is it worth a comment at the declaration of
l3_protocols warning about this assumption for when new protocol names
are added to the table?

Added a comment there. Though if abc comes before ab, that would 
still work, though entries have to be sorted that way then.

+
+return -1;
+}
+
+static int
+ebtablesCreateTmpRootAndSubChains(virBufferPtr buf,
+  const char *ifname,
+  virHashTablePtr chains, int direction)
+{
+int rc = 0, i;
+
+if (virHashSize(chains) != 0) {
+char **filter_names;
+
+ebtablesCreateTmpRootChain(buf, direction, ifname, 1);
+filter_names = (char **)virHashGetKeys(chains,
+   ebiptablesFilterOrderSort);

This area of code will be impacted by my comments on 1/10.


Conversion done.

+if (filter_names == NULL) {
+virReportOOMError();

Your implementation of virHashGetKeys already reported OOM error for all
error paths except for numElems  0; but that means that this call to
virReportOOMError() is either redundant (a second report), or misleading
(if numElems  0, you are not OOM, but have a programming bug on hand
for passing in an invalid hash table in the first place).

So I am not reporting an error anymore. If the hash table 'chains' was 
NULL, which would then also lead to '', it would not end up in this 
function at all but would have been intercepted earlier.

+rc = 1;

Can we get this updated to use -1 for failures, so that there's less to
clean up later when we fix this file to match libvirt conventions?


Did that now.
There will also be a patch converting all the nwfilter code to return 
'-1' upon failure. I got that in my queue, but only for later.

+goto err_exit;
+}
+for (i = 0; filter_names[i]; i++) {
+enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername(
+  filter_names[i]);
+if ((int)idx  0)
+continue;
+ebtablesCreateTmpSubChain(buf, direction, ifname, idx,
+  filter_names[i], 1);
+}
+virHashFreeKeys(chains, (void **)filter_names);

Oh, this reminds me of some additional feedback I meant to give on 1/10
- as part of the documentation you add, be sure to mention that the
returned array is only valid as long as the underlying hash table is not
modified (especially if you alter things to return in-hash pointers
rather than cloning keys) - adding or removing elements, or deleting the
hash table, will invalidate the returned key array.


I added that also as comment to the virHashGetItems() description.

@@ -3337,24 +3396,43 @@ ebiptablesApplyNewRules(virConnectPtr co
  int i;
  int cli_status;
  ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst;
-int chains_in = 0, chains_out = 0;

Re: [libvirt] [PATCH V4 03/10] Make filter creation in root table more flexible

2011-11-16 Thread Eric Blake 
On 10/26/2011 05:36 AM, Stefan Berger wrote:
 Use the previously introduced chain priorities to sort the chains for access
 from an interface's 'root' table and have them created in the proper order.
 This gets rid of a lot of code that was previously creating the chains in a 
 more hardcoded way.
 
 To determine what protocol a filter is used for evaluation do prefix-
 matching, i.e., the filter 'arp' is used to filter for the 'arp' protocol,
 'ipv4' for the 'ipv4' protocol and 'arp-xyz' will also be used to filter
 for the 'arp' protocol following the prefix 'arp' in its name.
 
 Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
 
 ---
  src/nwfilter/nwfilter_ebiptables_driver.c |  130 
 ++
  1 file changed, 98 insertions(+), 32 deletions(-)
 

  
 +static int
 +ebiptablesFilterOrderSort(const virHashKeyValuePairPtr a,
 +  const virHashKeyValuePairPtr b)
 +{
 +return *(virNWFilterChainPriority *)a-value -
 +   *(virNWFilterChainPriority *)b-value;
 +}

I tend to worry about someone passing INT_MAX and INT_MIN to a qsort
algorithm, then getting the wrong comparison sense because of integer
overflow when it uses straight subtraction; so I like to add a comment
explaining why I know that the valid input was capped and overflow is
impossible.

  
  static void
  iptablesCheckBridgeNFCallEnabled(bool isIPv6)
 @@ -3327,6 +3336,56 @@ iptablesCheckBridgeNFCallEnabled(bool is
  }
  }
  
 +/*
 + * Given a filtername determine the protocol it is used for evaluating
 + * We do prefix-matching to determine the protocol.
 + */
 +static enum l3_proto_idx
 +ebtablesGetProtoIdxByFiltername(const char *filtername)
 +{
 +enum l3_proto_idx idx;
 +
 +for (idx = 0; idx  L3_PROTO_LAST_IDX; idx++) {
 +if (STRPREFIX(filtername, l3_protocols[idx].val)) {
 +return idx;
 +}
 +}

This works as long as none of the entries in l3_protocols are a prefix
of any other entry; is it worth a comment at the declaration of
l3_protocols warning about this assumption for when new protocol names
are added to the table?

 +
 +return -1;
 +}
 +
 +static int
 +ebtablesCreateTmpRootAndSubChains(virBufferPtr buf,
 +  const char *ifname,
 +  virHashTablePtr chains, int direction)
 +{
 +int rc = 0, i;
 +
 +if (virHashSize(chains) != 0) {
 +char **filter_names;
 +
 +ebtablesCreateTmpRootChain(buf, direction, ifname, 1);
 +filter_names = (char **)virHashGetKeys(chains,
 +   ebiptablesFilterOrderSort);

This area of code will be impacted by my comments on 1/10.

 +if (filter_names == NULL) {
 +virReportOOMError();

Your implementation of virHashGetKeys already reported OOM error for all
error paths except for numElems  0; but that means that this call to
virReportOOMError() is either redundant (a second report), or misleading
(if numElems  0, you are not OOM, but have a programming bug on hand
for passing in an invalid hash table in the first place).

 +rc = 1;

Can we get this updated to use -1 for failures, so that there's less to
clean up later when we fix this file to match libvirt conventions?

 +goto err_exit;
 +}
 +for (i = 0; filter_names[i]; i++) {
 +enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername(
 +  filter_names[i]);
 +if ((int)idx  0)
 +continue;
 +ebtablesCreateTmpSubChain(buf, direction, ifname, idx,
 +  filter_names[i], 1);
 +}
 +virHashFreeKeys(chains, (void **)filter_names);

Oh, this reminds me of some additional feedback I meant to give on 1/10
- as part of the documentation you add, be sure to mention that the
returned array is only valid as long as the underlying hash table is not
modified (especially if you alter things to return in-hash pointers
rather than cloning keys) - adding or removing elements, or deleting the
hash table, will invalidate the returned key array.

 @@ -3337,24 +3396,43 @@ ebiptablesApplyNewRules(virConnectPtr co
  int i;
  int cli_status;
  ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst;
 -int chains_in = 0, chains_out = 0;
  virBuffer buf = VIR_BUFFER_INITIALIZER;
 +virHashTablePtr chains_in_set  = virHashCreate(10, NULL),
 +chains_out_set = virHashCreate(10, NULL);

Style-wise, I would list this as two declarations separated by ';',
rather than one declaration of two variables separated by ','.  That is,
I find it slightly hard to read stacked declarations that involve
multi-argument function calls as the initializers.

If patch 1/10 were used as-is, then this patch looks good except for a
few easy-to-fix nits.  But more likely, this will also need a v5 due to
rebasing on top of changes to how

[libvirt] [PATCH V4 03/10] Make filter creation in root table more flexible

2011-10-26 Thread Stefan Berger 
Use the previously introduced chain priorities to sort the chains for access
from an interface's 'root' table and have them created in the proper order.
This gets rid of a lot of code that was previously creating the chains in a 
more hardcoded way.

To determine what protocol a filter is used for evaluation do prefix-
matching, i.e., the filter 'arp' is used to filter for the 'arp' protocol,
'ipv4' for the 'ipv4' protocol and 'arp-xyz' will also be used to filter
for the 'arp' protocol following the prefix 'arp' in its name.

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

---
 src/nwfilter/nwfilter_ebiptables_driver.c |  130 ++
 1 file changed, 98 insertions(+), 32 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
@@ -2774,6 +2774,7 @@ ebtablesCreateTmpSubChain(virBufferPtr b
   int incoming,
   const char *ifname,
   enum l3_proto_idx protoidx,
+  const char *filtername,
   int stopOnError)
 {
 char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH];
@@ -2781,7 +2782,8 @@ ebtablesCreateTmpSubChain(virBufferPtr b
   : CHAINPREFIX_HOST_OUT_TEMP;
 
 PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname);
-PRINT_CHAIN(chain, chainPrefix, ifname, l3_protocols[protoidx].val);
+PRINT_CHAIN(chain, chainPrefix, ifname,
+(filtername) ? filtername : l3_protocols[protoidx].val);
 
 virBufferAsprintf(buf,
   CMD_DEF(%s -t %s -N %s) CMD_SEPARATOR
@@ -3288,6 +3290,13 @@ ebiptablesRuleOrderSort(const void *a, c
 return ((*insta)-priority - (*instb)-priority);
 }
 
+static int
+ebiptablesFilterOrderSort(const virHashKeyValuePairPtr a,
+  const virHashKeyValuePairPtr b)
+{
+return *(virNWFilterChainPriority *)a-value -
+   *(virNWFilterChainPriority *)b-value;
+}
 
 static void
 iptablesCheckBridgeNFCallEnabled(bool isIPv6)
@@ -3327,6 +3336,56 @@ iptablesCheckBridgeNFCallEnabled(bool is
 }
 }
 
+/*
+ * Given a filtername determine the protocol it is used for evaluating
+ * We do prefix-matching to determine the protocol.
+ */
+static enum l3_proto_idx
+ebtablesGetProtoIdxByFiltername(const char *filtername)
+{
+enum l3_proto_idx idx;
+
+for (idx = 0; idx  L3_PROTO_LAST_IDX; idx++) {
+if (STRPREFIX(filtername, l3_protocols[idx].val)) {
+return idx;
+}
+}
+
+return -1;
+}
+
+static int
+ebtablesCreateTmpRootAndSubChains(virBufferPtr buf,
+  const char *ifname,
+  virHashTablePtr chains, int direction)
+{
+int rc = 0, i;
+
+if (virHashSize(chains) != 0) {
+char **filter_names;
+
+ebtablesCreateTmpRootChain(buf, direction, ifname, 1);
+filter_names = (char **)virHashGetKeys(chains,
+   ebiptablesFilterOrderSort);
+if (filter_names == NULL) {
+virReportOOMError();
+rc = 1;
+goto err_exit;
+}
+for (i = 0; filter_names[i]; i++) {
+enum l3_proto_idx idx = ebtablesGetProtoIdxByFiltername(
+  filter_names[i]);
+if ((int)idx  0)
+continue;
+ebtablesCreateTmpSubChain(buf, direction, ifname, idx,
+  filter_names[i], 1);
+}
+virHashFreeKeys(chains, (void **)filter_names);
+}
+
+ err_exit:
+return rc;
+}
 
 static int
 ebiptablesApplyNewRules(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -3337,24 +3396,43 @@ ebiptablesApplyNewRules(virConnectPtr co
 int i;
 int cli_status;
 ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst;
-int chains_in = 0, chains_out = 0;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
+virHashTablePtr chains_in_set  = virHashCreate(10, NULL),
+chains_out_set = virHashCreate(10, NULL);
 bool haveIptables = false;
 bool haveIp6tables = false;
 
+if (!chains_in_set || !chains_out_set) {
+virReportOOMError();
+goto exit_free_sets;
+}
+
 if (nruleInstances  1  inst)
 qsort(inst, nruleInstances, sizeof(inst[0]), ebiptablesRuleOrderSort);
 
+/* scan the rules to see which chains need to be created */
 for (i = 0; i  nruleInstances; i++) {
 sa_assert (inst);
 if (inst[i]-ruleType == RT_EBTABLES) {
-if (inst[i]-chainprefix == CHAINPREFIX_HOST_IN_TEMP)
-chains_in  |= (1  inst[i]-neededProtocolChain);
-else
-chains_out |= (1  inst[i]-neededProtocolChain);
+const char