[libvirt] [PATCH V4 0/4] Support for multiple IP addresses using lists
This patch series builds on the previously posted patch series https://www.redhat.com/archives/libvir-list/2011-October/msg00912.html and introduces the capability to assign a list to a variable and have multiple rules instantiated, one for each item in the list. This means, that if for example a variable like IP has been assigned a list of values IP = [1.2.3.4, 5.6.7.8, 10.0.0.1] it will instantiate 3 rules, which in turn allows us to build filters that can evaluate multiple possible values per field, i.e., allow the filtering for multiple IP addresses (per interface). It would then need David Steven's patch for support of 'RETURN' (and 'CONTINUE') target(s). v4: - following Daniel Berrange's comments - changed (default) behavior of iterator v3: - following Daniel Berrange's comment regarding how a list of items should be represented in the XML v2: - reimplementation of iterator - other nits Regards, Stefan
NWFilters can be provided name-value pairs using the following XML notiation: <filterref filter='xyz'> <parameter name='PORT' value='80'/> <parameter name='VAL' value='abc'/> </filterref> The internal representation currently is so that a name is stored as a string and the value as well. This patch now addresses the value part of it and introduces a data structure for storing a value either as a simple value or as an array for later support of lists (provided in python-like notation ( [a,b,c] ). This patch adjusts all code that was handling the values in hash tables and makes it use the new data type. v4: - Fixed virNWFilterVarValueDelValue to maintain order of elements Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 2 src/conf/nwfilter_params.c | 291 ++++++++++++++++++++++++++++-- src/conf/nwfilter_params.h | 38 +++ src/libvirt_private.syms | 3 src/nwfilter/nwfilter_ebiptables_driver.c | 15 + src/nwfilter/nwfilter_gentech_driver.c | 27 ++ src/nwfilter/nwfilter_learnipaddr.c | 13 + 7 files changed, 368 insertions(+), 21 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 @@ -200,14 +200,25 @@ printVar(virNWFilterHashTablePtr vars, *done = 0; if ((item->flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { - char *val = (char *)virHashLookup(vars->hashTable, item->var); - if (!val) { + virNWFilterVarValuePtr varval; + const char *val; + + varval = virHashLookup(vars->hashTable, item->var); + if (!varval) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find value for '%s'"), item->var); return 1; } + val = virNWFilterVarValueGetSimple(varval); + if (!val) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot get simple value of '%s'"), + item->var); + return 1; + } + if (!virStrcpy(buf, val, bufsize)) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Buffer to small to print MAC address " Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -30,14 +30,271 @@ #include "datatypes.h" #include "nwfilter_params.h" #include "domain_conf.h" +#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_NWFILTER +static bool isValidVarValue(const char *value); + + +static void +virNWFilterVarValueFree(virNWFilterVarValuePtr val) +{ + unsigned i; + + if (!val) + return; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + VIR_FREE(val->u.simple.value); + break; + case NWFILTER_VALUE_TYPE_ARRAY: + for (i = 0; i < val->u.array.nValues; i++) + VIR_FREE(val->u.array.values[i]); + VIR_FREE(val->u.array.values); + break; + case NWFILTER_VALUE_TYPE_LAST: + break; + } + VIR_FREE(val); +} + +static virNWFilterVarValuePtr +virNWFilterVarValueCopy(const virNWFilterVarValuePtr val) +{ + virNWFilterVarValuePtr res; + unsigned i; + char *str; + + if (VIR_ALLOC(res) < 0) { + virReportOOMError(); + return NULL; + } + res->valType = val->valType; + + switch (res->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + if (val->u.simple.value) { + res->u.simple.value = strdup(val->u.simple.value); + if (!res->u.simple.value) + goto err_exit; + } + break; + case NWFILTER_VALUE_TYPE_ARRAY: + if (VIR_ALLOC_N(res->u.array.values, val->u.array.nValues)) + goto err_exit; + res->u.array.nValues = val->u.array.nValues; + for (i = 0; i < val->u.array.nValues; i++) { + str = strdup(val->u.array.values[i]); + if (!str) + goto err_exit; + res->u.array.values[i] = str; + } + break; + case NWFILTER_VALUE_TYPE_LAST: + break; + } + + return res; + +err_exit: + virReportOOMError(); + virNWFilterVarValueFree(res); + return NULL; +} + +void +virNWFilterVarValuePrint(virNWFilterVarValuePtr val, virBufferPtr buf) +{ + unsigned i; + char *item; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + virBufferAdd(buf, val->u.simple.value, -1); + break; + case NWFILTER_VALUE_TYPE_ARRAY: + virBufferAddLit(buf, "["); + for (i = 0; i < val->u.array.nValues; i++) { + if (i > 0) + virBufferAddLit(buf, ", "); + item = val->u.array.values[i]; + if (item) { + bool quote = false; + if (c_isspace(item[0]) || c_isspace(item[strlen(item)- 1 ])) + quote = true; + if (quote) + virBufferEscapeString(buf, "%s", "'"); + virBufferAdd(buf, val->u.array.values[i], -1); + if (quote) + virBufferEscapeString(buf, "%s", "'"); + } + } + virBufferAddLit(buf, "]"); + break; + case NWFILTER_VALUE_TYPE_LAST: + break; + } +} + +virNWFilterVarValuePtr +virNWFilterVarValueCreateSimple(const char *value, bool copy_value) +{ + virNWFilterVarValuePtr val; + + if (!isValidVarValue(value)) + return NULL; + + if (VIR_ALLOC(val) < 0) { + virReportOOMError(); + return NULL; + } + + val->valType = NWFILTER_VALUE_TYPE_SIMPLE; + if (copy_value) { + val->u.simple.value = strdup(value); + if (!val->u.simple.value) { + VIR_FREE(val); + virReportOOMError(); + return NULL; + } + } else + val->u.simple.value = (char *)value; + + return val; +} + +const char * +virNWFilterVarValueGetSimple(virNWFilterVarValuePtr val) +{ + if (val->valType == NWFILTER_VALUE_TYPE_SIMPLE) + return val->u.simple.value; + return NULL; +} + +const char * +virNWFilterVarValueGetNthValue(virNWFilterVarValuePtr val, unsigned int idx) +{ + const char *res = NULL; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + if (idx == 0) + res = val->u.simple.value; + break; + case NWFILTER_VALUE_TYPE_ARRAY: + if (idx < val->u.array.nValues) + res = val->u.array.values[idx]; + break; + case NWFILTER_VALUE_TYPE_LAST: + break; + } + + return res; +} + +unsigned int +virNWFilterVarValueGetCardinality(virNWFilterVarValuePtr val) +{ + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + return 1; + break; + case NWFILTER_VALUE_TYPE_ARRAY: + return val->u.array.nValues; + break; + case NWFILTER_VALUE_TYPE_LAST: + return 0; + } + return 0; +} + +bool +virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value) +{ + unsigned int i; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + return false; + + case NWFILTER_VALUE_TYPE_ARRAY: + for (i = 0; i < val->u.array.nValues; i++) { + if (STREQ(value, val->u.array.values[i])) { + VIR_FREE(val->u.array.values[i]); + + i++; + for (; i < val->u.array.nValues; i++) + val->u.array.values[i - 1] = val->u.array.values[i]; + + val->u.array.nValues--; + return true; + } + } + break; + + case NWFILTER_VALUE_TYPE_LAST: + break; + } + + return false; +} + +bool +virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, const char *value, + bool make_copy) +{ + char *tmp; + bool rc = false; + + if (make_copy) { + value = strdup(value); + if (!value) { + virReportOOMError(); + return false; + } + } + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + /* switch to array */ + tmp = val->u.simple.value; + if (VIR_ALLOC_N(val->u.array.values, 2) < 0) { + val->u.simple.value = tmp; + virReportOOMError(); + return false; + } + val->valType = NWFILTER_VALUE_TYPE_ARRAY; + val->u.array.nValues = 2; + val->u.array.values[0] = tmp; + val->u.array.values[1] = (char *)value; + rc = true; + break; + + case NWFILTER_VALUE_TYPE_ARRAY: + if (VIR_REALLOC_N(val->u.array.values, + val->u.array.nValues + 1) < 0) { + virReportOOMError(); + return false; + } + val->u.array.values[val->u.array.nValues] = (char *)value; + val->u.array.nValues += 1; + rc = true; + break; + + case NWFILTER_VALUE_TYPE_LAST: + break; + } + + return rc; +} + static void hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { - VIR_FREE(payload); + virNWFilterVarValueFree(payload); } @@ -56,7 +313,7 @@ hashDataFree(void *payload, const void * int virNWFilterHashTablePut(virNWFilterHashTablePtr table, const char *name, - char *val, + virNWFilterVarValuePtr val, int copyName) { if (!virHashLookup(table->hashTable, name)) { @@ -160,12 +417,12 @@ static void addToTable(void *payload, const void *name, void *data) { struct addToTableStruct *atts = (struct addToTableStruct *)data; - char *val; + virNWFilterVarValuePtr val; if (atts->errOccurred) return; - val = strdup((char *)payload); + val = virNWFilterVarValueCopy((virNWFilterVarValuePtr)payload); if (!val) { virReportOOMError(); atts->errOccurred = 1; @@ -177,7 +434,7 @@ addToTable(void *payload, const void *na _("Could not put variable '%s' into hashmap"), (const char *)name); atts->errOccurred = 1; - VIR_FREE(val); + virNWFilterVarValueFree(val); } } @@ -215,11 +472,17 @@ isValidVarValue(const char *value) return value[strspn(value, VALID_VARVALUE)] == 0; } +static virNWFilterVarValuePtr +virNWFilterParseVarValue(const char *val) +{ + return virNWFilterVarValueCreateSimple(val, true); +} virNWFilterHashTablePtr virNWFilterParseParamAttributes(xmlNodePtr cur) { char *nam, *val; + virNWFilterVarValuePtr value; virNWFilterHashTablePtr table = virNWFilterHashTableCreate(0); if (!table) { @@ -234,20 +497,24 @@ virNWFilterParseParamAttributes(xmlNodeP if (xmlStrEqual(cur->name, BAD_CAST "parameter")) { nam = virXMLPropString(cur, "name"); val = virXMLPropString(cur, "value"); + value = NULL; if (nam != NULL && val != NULL) { if (!isValidVarName(nam)) goto skip_entry; - if (!isValidVarValue(nam)) + value = virNWFilterParseVarValue(val); + if (!value) goto skip_entry; - if (virNWFilterHashTablePut(table, nam, val, 1)) { + if (virNWFilterHashTablePut(table, nam, value, 1)) { VIR_FREE(nam); VIR_FREE(val); + virNWFilterVarValueFree(value); virNWFilterHashTableFree(table); return NULL; } - val = NULL; + value = NULL; } skip_entry: + virNWFilterVarValueFree(value); VIR_FREE(nam); VIR_FREE(val); } @@ -268,11 +535,13 @@ static void _formatParameterAttrs(void *payload, const void *name, void *data) { struct formatterParam *fp = (struct formatterParam *)data; + virNWFilterVarValuePtr value = payload; - virBufferAsprintf(fp->buf, "%s<parameter name='%s' value='%s'/>\n", + virBufferAsprintf(fp->buf, "%s<parameter name='%s' value='", fp->indent, - (const char *)name, - (char *)payload); + (const char *)name); + virNWFilterVarValuePrint(value, fp->buf); + virBufferAddLit(fp->buf, "'/>\n"); } Index: libvirt-acl/src/conf/nwfilter_params.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.h +++ libvirt-acl/src/conf/nwfilter_params.h @@ -23,6 +23,42 @@ # define NWFILTER_PARAMS_H # include "hash.h" +# include "buf.h" + +enum virNWFilterVarValueType { + NWFILTER_VALUE_TYPE_SIMPLE, + NWFILTER_VALUE_TYPE_ARRAY, + + NWFILTER_VALUE_TYPE_LAST +}; + +typedef struct _virNWFilterVarValue virNWFilterVarValue; +typedef virNWFilterVarValue *virNWFilterVarValuePtr; +struct _virNWFilterVarValue { + enum virNWFilterVarValueType valType; + union { + struct { + char *value; + } simple; + struct { + char **values; + unsigned nValues; + } array; + } u; +}; + +virNWFilterVarValuePtr virNWFilterVarValueCreateSimple(const char *, + bool copy_value); +const char *virNWFilterVarValueGetSimple(virNWFilterVarValuePtr val); +const char *virNWFilterVarValueGetNthValue(virNWFilterVarValuePtr val, + unsigned int idx); +unsigned int virNWFilterVarValueGetCardinality(virNWFilterVarValuePtr val); +bool virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, + const char *value); +bool virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, + const char *value, + bool make_copy); +void virNWFilterVarValuePrint(virNWFilterVarValuePtr val, virBufferPtr buf); typedef struct _virNWFilterHashTable virNWFilterHashTable; typedef virNWFilterHashTable *virNWFilterHashTablePtr; @@ -42,7 +78,7 @@ virNWFilterHashTablePtr virNWFilterHashT void virNWFilterHashTableFree(virNWFilterHashTablePtr table); int virNWFilterHashTablePut(virNWFilterHashTablePtr table, const char *name, - char *val, + virNWFilterVarValuePtr val, int freeName); int virNWFilterHashTableRemoveEntry(virNWFilterHashTablePtr table, const char *name); Index: libvirt-acl/src/conf/domain_conf.c =================================================================== --- libvirt-acl.orig/src/conf/domain_conf.c +++ libvirt-acl/src/conf/domain_conf.c @@ -3182,7 +3182,7 @@ virDomainNetDefParseXML(virCapsPtr caps, event_idx = virXMLPropString(cur, "event_idx"); } else if (xmlStrEqual (cur->name, BAD_CAST "filterref")) { filter = virXMLPropString(cur, "filter"); - VIR_FREE(filterparams); + virNWFilterHashTableFree(filterparams); filterparams = virNWFilterParseParamAttributes(cur); } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -147,10 +147,17 @@ virNWFilterVarHashmapAddStdValues(virNWF char *macaddr, char *ipaddr) { + virNWFilterVarValue *val; + if (macaddr) { + val = virNWFilterVarValueCreateSimple(macaddr, false); + if (!val) { + virReportOOMError(); + return 1; + } if (virHashAddEntry(table->hashTable, NWFILTER_STD_VAR_MAC, - macaddr) < 0) { + val) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'MAC' to hashmap")); return 1; @@ -158,9 +165,14 @@ virNWFilterVarHashmapAddStdValues(virNWF } if (ipaddr) { + val = virNWFilterVarValueCreateSimple(ipaddr, false); + if (!val) { + virReportOOMError(); + return 1; + } if (virHashAddEntry(table->hashTable, NWFILTER_STD_VAR_IP, - ipaddr) < 0) { + val) < 0) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add variable 'IP' to hashmap")); return 1; @@ -491,6 +503,7 @@ virNWFilterDetermineMissingVarsRec(virCo int rc = 0; int i, j; virNWFilterDefPtr next_filter; + virNWFilterVarValuePtr val; for (i = 0; i < filter->nentries; i++) { virNWFilterRuleDefPtr rule = filter->filterEntries[i]->rule; @@ -499,10 +512,18 @@ virNWFilterDetermineMissingVarsRec(virCo /* check all variables of this rule */ for (j = 0; j < rule->nvars; j++) { if (!virHashLookup(vars->hashTable, rule->vars[j])) { + val = virNWFilterVarValueCreateSimple("1", true); + if (!val) { + virReportOOMError(); + rc = 1; + break; + } virNWFilterHashTablePut(missing_vars, rule->vars[j], - strdup("1"), 1); + val, 1); } } + if (rc) + break; } else if (inc) { VIR_DEBUG("Following filter %s\n", inc->filterref); obj = virNWFilterObjFindByName(&driver->nwfilters, inc->filterref); Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -313,10 +313,14 @@ virNWFilterDeregisterLearnReq(int ifinde static int virNWFilterAddIpAddrForIfname(const char *ifname, char *addr) { int ret; + virNWFilterVarValuePtr val = virNWFilterVarValueCreateSimple(addr, false); + + if (!val) + return 1; virMutexLock(&ipAddressMapLock); - ret = virNWFilterHashTablePut(ipAddressMap, ifname, addr, 1); + ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1); virMutexUnlock(&ipAddressMapLock); @@ -339,7 +343,7 @@ virNWFilterDelIpAddrForIfname(const char const char * virNWFilterGetIpAddrForIfname(const char *ifname) { - const char *res; + virNWFilterVarValuePtr res; virMutexLock(&ipAddressMapLock); @@ -347,7 +351,10 @@ virNWFilterGetIpAddrForIfname(const char virMutexUnlock(&ipAddressMapLock); - return res; + if (res) + return virNWFilterVarValueGetSimple(res); + + return NULL; } Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -885,6 +885,9 @@ virNWFilterHashTableFree; virNWFilterHashTablePut; virNWFilterHashTablePutAll; virNWFilterHashTableRemoveEntry; +virNWFilterVarValueCreateSimple; +virNWFilterVarValueGetSimple; +virNWFilterVarValuePrint; # pci.h
On 10/27/2011 03:07 PM, Stefan Berger wrote:
NWFilters can be provided name-value pairs using the following XML notiation:
<filterref filter='xyz'> <parameter name='PORT' value='80'/> <parameter name='VAL' value='abc'/> </filterref>
The internal representation currently is so that a name is stored as a string and the value as well. This patch now addresses the value part of it and introduces a data structure for storing a value either as a simple value or as an array for later support of lists (provided in python-like notation ( [a,b,c] ).
This patch adjusts all code that was handling the values in hash tables and makes it use the new data type.
v4: - Fixed virNWFilterVarValueDelValue to maintain order of elements
+ +void +virNWFilterVarValuePrint(virNWFilterVarValuePtr val, virBufferPtr buf) +{ + unsigned i; + char *item; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + virBufferAdd(buf, val->u.simple.value, -1); + break; + case NWFILTER_VALUE_TYPE_ARRAY: + virBufferAddLit(buf, "["); + for (i = 0; i< val->u.array.nValues; i++) { + if (i> 0) + virBufferAddLit(buf, ", "); + item = val->u.array.values[i]; + if (item) { + bool quote = false; + if (c_isspace(item[0]) || c_isspace(item[strlen(item)- 1 ])) + quote = true; + if (quote) + virBufferEscapeString(buf, "%s", "'"); + virBufferAdd(buf, val->u.array.values[i], -1); + if (quote) + virBufferEscapeString(buf, "%s", "'"); + } + } + virBufferAddLit(buf, "]");
This needs to be fixed, per Daniel's comments here: https://www.redhat.com/archives/libvir-list/2011-October/msg01105.html We'll need a v5, but as long as I've started reviewing this series, I'll keep going...
+ val->valType = NWFILTER_VALUE_TYPE_SIMPLE; + if (copy_value) { + val->u.simple.value = strdup(value); + if (!val->u.simple.value) { + VIR_FREE(val); + virReportOOMError(); + return NULL; + } + } else + val->u.simple.value = (char *)value;
Style - with if-else, if one branch needs {}, then you should use {} on both branches. Casting away const is risky - it does mean that the compiler can't enforce someone who accidentally calls virNWFilterVarValueCreateSimple("string_lit", false) Could we instead split the decision by having two functions, correctly typed? virNWFilterVarValueTransferSimple(char *) - reuse virNWFilterVarValueCopySimple(const char *) - copy
+ +bool +virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value) +{ + unsigned int i; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + return false; + + case NWFILTER_VALUE_TYPE_ARRAY: + for (i = 0; i< val->u.array.nValues; i++) { + if (STREQ(value, val->u.array.values[i])) { + VIR_FREE(val->u.array.values[i]); + + i++; + for (; i< val->u.array.nValues; i++) + val->u.array.values[i - 1] = val->u.array.values[i];
Would memmove() be more efficient here?
+ +bool +virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, const char *value, + bool make_copy) +{ + char *tmp; + bool rc = false; + + if (make_copy) { + value = strdup(value);
Now you've locked the just-malloc'd value into being const char*. I would have gone through an intermediate variable, 'char *', so that you can call helper functions without having to cast away const.
+ if (!value) { + virReportOOMError(); + return false; + } + } + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + /* switch to array */ + tmp = val->u.simple.value; + if (VIR_ALLOC_N(val->u.array.values, 2)< 0) { + val->u.simple.value = tmp; + virReportOOMError(); + return false; + } + val->valType = NWFILTER_VALUE_TYPE_ARRAY; + val->u.array.nValues = 2; + val->u.array.values[0] = tmp; + val->u.array.values[1] = (char *)value;
If the user didn't pass make_copy, this is casting away const; are we up for the maintenance cost of making sure that is always safe?
+ rc = true; + break; + + case NWFILTER_VALUE_TYPE_ARRAY: + if (VIR_REALLOC_N(val->u.array.values, + val->u.array.nValues + 1)< 0) {
VIR_EXPAND_N might be better here; as it is a bit more structured than VIR_REALLOC_N at guaranteeing new elements start life zero-initialized.
+typedef struct _virNWFilterVarValue virNWFilterVarValue; +typedef virNWFilterVarValue *virNWFilterVarValuePtr; +struct _virNWFilterVarValue { + enum virNWFilterVarValueType valType; + union { + struct { + char *value; + } simple; + struct { + char **values; + unsigned nValues;
size_t tends to be better for array length.
Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -147,10 +147,17 @@ virNWFilterVarHashmapAddStdValues(virNWF char *macaddr, char *ipaddr) { + virNWFilterVarValue *val; + if (macaddr) { + val = virNWFilterVarValueCreateSimple(macaddr, false); + if (!val) { + virReportOOMError(); + return 1; + }
Pre-existing, but we should probably convert this file to use a return convention of -1 for error.
@@ -499,10 +512,18 @@ virNWFilterDetermineMissingVarsRec(virCo /* check all variables of this rule */ for (j = 0; j< rule->nvars; j++) { if (!virHashLookup(vars->hashTable, rule->vars[j])) { + val = virNWFilterVarValueCreateSimple("1", true); + if (!val) { + virReportOOMError(); + rc = 1; + break; + } virNWFilterHashTablePut(missing_vars, rule->vars[j], - strdup("1"), 1); + val, 1);
Nice bug fix on the side, since the old code failed to check for strdup failure. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/28/2011 05:00 PM, Eric Blake wrote:
On 10/27/2011 03:07 PM, Stefan Berger wrote:
+ +void +virNWFilterVarValuePrint(virNWFilterVarValuePtr val, virBufferPtr buf) +{ + unsigned i; + char *item; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + virBufferAdd(buf, val->u.simple.value, -1); + break; + case NWFILTER_VALUE_TYPE_ARRAY: + virBufferAddLit(buf, "["); + for (i = 0; i< val->u.array.nValues; i++) { + if (i> 0) + virBufferAddLit(buf, ", "); + item = val->u.array.values[i]; + if (item) { + bool quote = false; + if (c_isspace(item[0]) || c_isspace(item[strlen(item)- 1 ])) + quote = true; + if (quote) + virBufferEscapeString(buf, "%s", "'"); + virBufferAdd(buf, val->u.array.values[i], -1); + if (quote) + virBufferEscapeString(buf, "%s", "'"); + } + } + virBufferAddLit(buf, "]");
This needs to be fixed, per Daniel's comments here: https://www.redhat.com/archives/libvir-list/2011-October/msg01105.html
Removed now.
+ val->valType = NWFILTER_VALUE_TYPE_SIMPLE; + if (copy_value) { + val->u.simple.value = strdup(value); + if (!val->u.simple.value) { + VIR_FREE(val); + virReportOOMError(); + return NULL; + } + } else + val->u.simple.value = (char *)value;
Style - with if-else, if one branch needs {}, then you should use {} on both branches.
Casting away const is risky - it does mean that the compiler can't enforce someone who accidentally calls virNWFilterVarValueCreateSimple("string_lit", false)
Could we instead split the decision by having two functions, correctly typed?
virNWFilterVarValueTransferSimple(char *) - reuse virNWFilterVarValueCopySimple(const char *) - copy
Truly this was not a good choice. The new functions are: virNWFilterVarValuePtr virNWFilterVarValueCreateSimple(char *); virNWFilterVarValuePtr virNWFilterVarValueCreateSimpleCopyValue(const char *);
+ +bool +virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value) +{ + unsigned int i; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + return false; + + case NWFILTER_VALUE_TYPE_ARRAY: + for (i = 0; i< val->u.array.nValues; i++) { + if (STREQ(value, val->u.array.values[i])) { + VIR_FREE(val->u.array.values[i]); + + i++; + for (; i< val->u.array.nValues; i++) + val->u.array.values[i - 1] = val->u.array.values[i];
Would memmove() be more efficient here?
I removed this function for now since there's no caller at the moment.
+ +bool +virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, const char *value, + bool make_copy) +{ + char *tmp; + bool rc = false; + + if (make_copy) { + value = strdup(value);
Now you've locked the just-malloc'd value into being const char*. I would have gone through an intermediate variable, 'char *', so that you can call helper functions without having to cast away const.
New function: bool virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, char *value);
+ if (!value) { + virReportOOMError(); + return false; + } + } + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + /* switch to array */ + tmp = val->u.simple.value; + if (VIR_ALLOC_N(val->u.array.values, 2)< 0) { + val->u.simple.value = tmp; + virReportOOMError(); + return false; + } + val->valType = NWFILTER_VALUE_TYPE_ARRAY; + val->u.array.nValues = 2; + val->u.array.values[0] = tmp; + val->u.array.values[1] = (char *)value;
If the user didn't pass make_copy, this is casting away const; are we up for the maintenance cost of making sure that is always safe?
+ rc = true; + break; + + case NWFILTER_VALUE_TYPE_ARRAY: + if (VIR_REALLOC_N(val->u.array.values, + val->u.array.nValues + 1)< 0) {
VIR_EXPAND_N might be better here; as it is a bit more structured than VIR_REALLOC_N at guaranteeing new elements start life zero-initialized.
Converted.
+typedef struct _virNWFilterVarValue virNWFilterVarValue; +typedef virNWFilterVarValue *virNWFilterVarValuePtr; +struct _virNWFilterVarValue { + enum virNWFilterVarValueType valType; + union { + struct { + char *value; + } simple; + struct { + char **values; + unsigned nValues;
size_t tends to be better for array length.
Fixed.
Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -147,10 +147,17 @@ virNWFilterVarHashmapAddStdValues(virNWF char *macaddr, char *ipaddr) { + virNWFilterVarValue *val; + if (macaddr) { + val = virNWFilterVarValueCreateSimple(macaddr, false); + if (!val) { + virReportOOMError(); + return 1; + }
Pre-existing, but we should probably convert this file to use a return convention of -1 for error.
I'll put this on my TODO list.
@@ -499,10 +512,18 @@ virNWFilterDetermineMissingVarsRec(virCo /* check all variables of this rule */ for (j = 0; j< rule->nvars; j++) { if (!virHashLookup(vars->hashTable, rule->vars[j])) { + val = virNWFilterVarValueCreateSimple("1", true); + if (!val) { + virReportOOMError(); + rc = 1; + break; + } virNWFilterHashTablePut(missing_vars, rule->vars[j], - strdup("1"), 1); + val, 1);
Nice bug fix on the side, since the old code failed to check for strdup failure.
:-) Stefan
This patch extends the NWFilter driver for Linux (ebiptables) to create rules for each member of a previously introduced list. If for example an attribute value (internally) looks like this: IP = [10.0.0.1, 10.0.0.2, 10.0.0.3] then 3 rules will be generated for a rule accessing the variable 'IP', one for each member of the list. The effect of this is that this now allows for filtering for multiple values in one field. This can then be used to support for filtering/allowing of multiple IP addresses per interface. An interator is introduced that extracts each member of a list and puts it into a hash table which then is passed to the function creating a rule. For the above example the iterator would cause 3 loops. v4: - changed iterator to access multiple lists' elements at the same index; previous behavior was to produce all combinations of all the elements in the given lists v2: - pass the iterator all the way to the function that accesses the hash table and provide a function to pick the value of a variable that is reflected by the current state of the iterator Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/conf/nwfilter_params.c | 205 ++++++++++++++++++++++++++++++ src/conf/nwfilter_params.h | 27 +++ src/libvirt_private.syms | 4 src/nwfilter/nwfilter_ebiptables_driver.c | 81 +++++++---- 4 files changed, 290 insertions(+), 27 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 @@ -192,7 +192,7 @@ static const struct ushort_map l3_protoc static int -printVar(virNWFilterHashTablePtr vars, +printVar(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, nwItemDescPtr item, int *done) @@ -200,22 +200,11 @@ printVar(virNWFilterHashTablePtr vars, *done = 0; if ((item->flags & NWFILTER_ENTRY_ITEM_FLAG_HAS_VAR)) { - virNWFilterVarValuePtr varval; const char *val; - varval = virHashLookup(vars->hashTable, item->var); - if (!varval) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find value for '%s'"), - item->var); - return 1; - } - - val = virNWFilterVarValueGetSimple(varval); + val = virNWFilterVarCombIterGetVarValue(vars, item->var); if (!val) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot get simple value of '%s'"), - item->var); + /* error has been reported */ return 1; } @@ -234,7 +223,7 @@ printVar(virNWFilterHashTablePtr vars, static int -_printDataType(virNWFilterHashTablePtr vars, +_printDataType(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, nwItemDescPtr item, bool asHex) @@ -329,7 +318,7 @@ _printDataType(virNWFilterHashTablePtr v static int -printDataType(virNWFilterHashTablePtr vars, +printDataType(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, nwItemDescPtr item) { @@ -338,7 +327,7 @@ printDataType(virNWFilterHashTablePtr va static int -printDataTypeAsHex(virNWFilterHashTablePtr vars, +printDataTypeAsHex(virNWFilterVarCombIterPtr vars, char *buf, int bufsize, nwItemDescPtr item) { @@ -406,7 +395,7 @@ ebiptablesAddRuleInst(virNWFilterRuleIns static int ebtablesHandleEthHdr(virBufferPtr buf, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, ethHdrDataDefPtr ethHdr, bool reverse) { @@ -884,7 +873,7 @@ iptablesInstCommand(virBufferPtr buf, static int iptablesHandleSrcMacAddr(virBufferPtr buf, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, nwItemDescPtr srcMacAddr, int directionIn, bool *srcmacskipped) @@ -921,7 +910,7 @@ err_exit: static int iptablesHandleIpHdr(virBufferPtr buf, virBufferPtr afterStateMatch, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, ipHdrDataDefPtr ipHdr, int directionIn, bool *skipRule, bool *skipMatch, @@ -1095,7 +1084,7 @@ err_exit: static int iptablesHandlePortData(virBufferPtr buf, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, portDataDefPtr portData, int directionIn) { @@ -1201,7 +1190,7 @@ _iptablesCreateRuleInstance(int directio virNWFilterDefPtr nwfilter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, virNWFilterRuleInstPtr res, const char *match, bool defMatch, const char *accept_target, @@ -1687,7 +1676,7 @@ static int iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, virNWFilterRuleInstPtr res, bool isIPv6) { @@ -1812,7 +1801,7 @@ static int iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, virNWFilterRuleInstPtr res, bool isIPv6) { @@ -1937,7 +1926,7 @@ ebtablesCreateRuleInstance(char chainPre virNWFilterDefPtr nwfilter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, virNWFilterRuleInstPtr res, bool reverse) { @@ -2429,7 +2418,7 @@ ebiptablesCreateRuleInstance(virConnectP virNWFilterDefPtr nwfilter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars, + virNWFilterVarCombIterPtr vars, virNWFilterRuleInstPtr res) { int rc = 0; @@ -2513,6 +2502,44 @@ ebiptablesCreateRuleInstance(virConnectP return rc; } +static int +ebiptablesCreateRuleInstanceIterate( + virConnectPtr conn ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + virNWFilterDefPtr nwfilter, + virNWFilterRuleDefPtr rule, + const char *ifname, + virNWFilterHashTablePtr vars, + virNWFilterRuleInstPtr res) +{ + int rc = 0; + virNWFilterVarCombIterPtr vciter; + + /* rule->vars holds all the variables names that this rule will access. + * iterate over all combinations of the variables' values and instantiate + * the filtering rule with each combination. + */ + vciter = virNWFilterVarCombIterCreate(vars, rule->vars, rule->nvars); + if (!vciter) + return 1; + + do { + rc = ebiptablesCreateRuleInstance(conn, + nettype, + nwfilter, + rule, + ifname, + vciter, + res); + if (rc) + break; + vciter = virNWFilterVarCombIterNext(vciter); + } while (vciter != NULL); + + virNWFilterVarCombIterFree(vciter); + + return rc; +} static int ebiptablesFreeRuleInstance(void *_inst) @@ -3896,7 +3923,7 @@ virNWFilterTechDriver ebiptables_driver .init = ebiptablesDriverInit, .shutdown = ebiptablesDriverShutdown, - .createRuleInstance = ebiptablesCreateRuleInstance, + .createRuleInstance = ebiptablesCreateRuleInstanceIterate, .applyNewRules = ebiptablesApplyNewRules, .tearNewRules = ebiptablesTearNewRules, .tearOldRules = ebiptablesTearOldRules, Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -291,6 +291,211 @@ virNWFilterVarValueAddValue(virNWFilterV return rc; } +void +virNWFilterVarCombIterFree(virNWFilterVarCombIterPtr ci) +{ + unsigned int i; + + if (!ci) + return; + + for (i = 0; i < ci->nIter; i++) + VIR_FREE(ci->iter[i].varNames); + + VIR_FREE(ci); +} + +static int +virNWFilterVarCombIterGetIndexByIterId(virNWFilterVarCombIterPtr ci, + unsigned int iterId) +{ + unsigned int i; + + for (i = 0; i < ci->nIter; i++) + if (ci->iter[i].iterId == iterId) + return i; + + return -1; +} + +static void +virNWFilterVarCombIterEntryInit(virNWFilterVarCombIterEntryPtr cie, + unsigned int iterId) +{ + memset(cie, 0, sizeof(*cie)); + cie->iterId = iterId; +} + +static int +virNWFilterVarCombIterAddVariable(virNWFilterVarCombIterEntryPtr cie, + virNWFilterHashTablePtr hash, + const char *varName) +{ + virNWFilterVarValuePtr varValue; + unsigned int cardinality; + + varValue = virHashLookup(hash->hashTable, varName); + if (varValue == NULL) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find value for variable '%s'"), + varName); + return 1; + } + + cardinality = virNWFilterVarValueGetCardinality(varValue); + + if (cie->nVarNames == 0) { + cie->maxValue = cardinality - 1; + } else { + if (cie->maxValue != cardinality - 1) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Cardinality of list items must be " + "the same for processing them in " + "parallel")); + return 1; + } + } + + if (VIR_REALLOC_N(cie->varNames, cie->nVarNames + 1) < 0) { + virReportOOMError(); + return 1; + } + + cie->varNames[cie->nVarNames] = varName; + cie->nVarNames++; + + return 0; +} + +/* + * Create an iterator over the contents of the given variables. All variables + * must have entries in the hash table. + * The iterator that is created processes all given variables in parallel, + * meaning it will access $ITEM1[0] and $ITEM2[0] then $ITEM1[1] and $ITEM2[1] + * upto $ITEM1[n] and $ITEM2[n]. For this to work, the cardinality of all + * processed lists must be the same. + * The notation $ITEM1 and $ITEM2 (in one rule) therefore will always have to + * process the items in parallel. This could be an implicit notiation for + * $ITEM1[@0] and $ITEM2[@0] to 'lock' the two together. Future notations of + * $ITEM1[@1] and $ITEM2[@2] will make them be processed independently, + * which then would cause all combinations of the items of the two lists to + * be created. + */ +virNWFilterVarCombIterPtr +virNWFilterVarCombIterCreate(virNWFilterHashTablePtr hash, + char * const *vars, unsigned int nVars) +{ + virNWFilterVarCombIterPtr res; + unsigned int i, iterId; + int iterIdx; + + if (VIR_ALLOC_VAR(res, virNWFilterVarCombIterEntry, nVars) < 0) { + virReportOOMError(); + return NULL; + } + + res->hashTable = hash; + + /* create the default iterator to support @0 */ + iterId = 0; + + res->nIter = 1; + virNWFilterVarCombIterEntryInit(&res->iter[0], iterId); + + for (i = 0; i < nVars; i++) { + + /* currently always access @0 */ + iterId = 0; + + iterIdx = virNWFilterVarCombIterGetIndexByIterId(res, iterId); + if (iterIdx < 0) { + /* future: create new iterator. for now it's a bug */ + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find iterator with id %u\n"), + iterId); + goto err_exit; + } + + if (virNWFilterVarCombIterAddVariable(&res->iter[iterIdx], + hash, vars[i])) + goto err_exit; + } + + return res; + +err_exit: + virNWFilterVarCombIterFree(res); + return NULL; +} + +virNWFilterVarCombIterPtr +virNWFilterVarCombIterNext(virNWFilterVarCombIterPtr ci) +{ + unsigned int i; + + for (i = 0; i < ci->nIter; i++) { + ci->iter[i].curValue++; + if (ci->iter[i].curValue <= ci->iter[i].maxValue) + break; + else + ci->iter[i].curValue = 0; + } + + if (ci->nIter == i) { + virNWFilterVarCombIterFree(ci); + return NULL; + } + + return ci; +} + +const char * +virNWFilterVarCombIterGetVarValue(virNWFilterVarCombIterPtr ci, + const char *varName) +{ + unsigned int i; + bool found = false; + const char *res = NULL; + virNWFilterVarValuePtr value; + unsigned int iterId; + + /* currently always accessing iter @0 */ + iterId = 0; + + for (i = 0; i < ci->iter[iterId].nVarNames; i++) { + if (STREQ(ci->iter[iterId].varNames[i], varName)) { + found = true; + break; + } + } + + if (!found) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find variable '%s' in iterator"), + varName); + return NULL; + } + + value = virHashLookup(ci->hashTable->hashTable, varName); + if (!value) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find value for variable '%s'"), + varName); + return NULL; + } + + res = virNWFilterVarValueGetNthValue(value, ci->iter[iterId].curValue); + if (!res) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get nth (%u) value of " + "variable '%s'"), + ci->iter[iterId].curValue, varName); + return NULL; + } + + return res; +} + static void hashDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { Index: libvirt-acl/src/conf/nwfilter_params.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.h +++ libvirt-acl/src/conf/nwfilter_params.h @@ -91,4 +91,31 @@ int virNWFilterHashTablePutAll(virNWFilt # define VALID_VARVALUE \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:" +typedef struct _virNWFilterVarCombIterEntry virNWFilterVarCombIterEntry; +typedef virNWFilterVarCombIterEntry *virNWFilterVarCombIterEntryPtr; +struct _virNWFilterVarCombIterEntry { + unsigned int iterId; + const char **varNames; + unsigned int nVarNames; + unsigned int maxValue; + unsigned int curValue; +}; + +typedef struct _virNWFilterVarCombIter virNWFilterVarCombIter; +typedef virNWFilterVarCombIter *virNWFilterVarCombIterPtr; +struct _virNWFilterVarCombIter { + virNWFilterHashTablePtr hashTable; + virNWFilterVarCombIterEntry iter[1]; + unsigned int nIter; +}; +virNWFilterVarCombIterPtr virNWFilterVarCombIterCreate( + virNWFilterHashTablePtr hash, + char * const *vars, unsigned int nVars); + +void virNWFilterVarCombIterFree(virNWFilterVarCombIterPtr ci); +virNWFilterVarCombIterPtr virNWFilterVarCombIterNext( + virNWFilterVarCombIterPtr ci); +const char *virNWFilterVarCombIterGetVarValue(virNWFilterVarCombIterPtr ci, + const char *varname); + #endif /* NWFILTER_PARAMS_H */ Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -885,6 +885,10 @@ virNWFilterHashTableFree; virNWFilterHashTablePut; virNWFilterHashTablePutAll; virNWFilterHashTableRemoveEntry; +virNWFilterVarCombIterCreate; +virNWFilterVarCombIterFree; +virNWFilterVarCombIterGetVarValue; +virNWFilterVarCombIterNext; virNWFilterVarValueCreateSimple; virNWFilterVarValueGetSimple; virNWFilterVarValuePrint;
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch extends the NWFilter driver for Linux (ebiptables) to create rules for each member of a previously introduced list. If for example an attribute value (internally) looks like this:
IP = [10.0.0.1, 10.0.0.2, 10.0.0.3]
then 3 rules will be generated for a rule accessing the variable 'IP', one for each member of the list. The effect of this is that this now allows for filtering for multiple values in one field. This can then be used to support for filtering/allowing of multiple IP addresses per interface.
An interator is introduced that extracts each member of a list and
s/interator/iterator/
puts it into a hash table which then is passed to the function creating a rule. For the above example the iterator would cause 3 loops.
+ebiptablesCreateRuleInstanceIterate( + virConnectPtr conn ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + virNWFilterDefPtr nwfilter, + virNWFilterRuleDefPtr rule, + const char *ifname, + virNWFilterHashTablePtr vars, + virNWFilterRuleInstPtr res) +{ + int rc = 0; + virNWFilterVarCombIterPtr vciter; + + /* rule->vars holds all the variables names that this rule will access. + * iterate over all combinations of the variables' values and instantiate + * the filtering rule with each combination. + */ + vciter = virNWFilterVarCombIterCreate(vars, rule->vars, rule->nvars); + if (!vciter) + return 1;
Shouldn't we go with the more typical convention of -1 on error?
+static int +virNWFilterVarCombIterAddVariable(virNWFilterVarCombIterEntryPtr cie, + virNWFilterHashTablePtr hash, + const char *varName) +{ + virNWFilterVarValuePtr varValue; + unsigned int cardinality; + + varValue = virHashLookup(hash->hashTable, varName); + if (varValue == NULL) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find value for variable '%s'"), + varName); + return 1; + }
Again.
+ + if (VIR_REALLOC_N(cie->varNames, cie->nVarNames + 1)< 0) { + virReportOOMError(); + return 1;
Here too.
+ } + + cie->varNames[cie->nVarNames] = varName; + cie->nVarNames++; + + return 0; +} + +/* + * Create an iterator over the contents of the given variables. All variables + * must have entries in the hash table. + * The iterator that is created processes all given variables in parallel, + * meaning it will access $ITEM1[0] and $ITEM2[0] then $ITEM1[1] and $ITEM2[1] + * upto $ITEM1[n] and $ITEM2[n]. For this to work, the cardinality of all
s/upto/up to/
+ * processed lists must be the same. + * The notation $ITEM1 and $ITEM2 (in one rule) therefore will always have to + * process the items in parallel. This could be an implicit notiation for
s/notiation/notation/
+typedef struct _virNWFilterVarCombIter virNWFilterVarCombIter; +typedef virNWFilterVarCombIter *virNWFilterVarCombIterPtr; +struct _virNWFilterVarCombIter { + virNWFilterHashTablePtr hashTable; + virNWFilterVarCombIterEntry iter[1];
1-element arrays look odd,
+ unsigned int nIter; +};
especially when they aren't the last element of a struct. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/28/2011 05:26 PM, Eric Blake wrote:
On 10/27/2011 03:07 PM, Stefan Berger wrote:
puts it into a hash table which then is passed to the function creating a rule. For the above example the iterator would cause 3 loops.
+ebiptablesCreateRuleInstanceIterate( + virConnectPtr conn ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + virNWFilterDefPtr nwfilter, + virNWFilterRuleDefPtr rule, + const char *ifname, + virNWFilterHashTablePtr vars, + virNWFilterRuleInstPtr res) +{ + int rc = 0; + virNWFilterVarCombIterPtr vciter; + + /* rule->vars holds all the variables names that this rule will access. + * iterate over all combinations of the variables' values and instantiate + * the filtering rule with each combination. + */ + vciter = virNWFilterVarCombIterCreate(vars, rule->vars, rule->nvars); + if (!vciter) + return 1;
Shouldn't we go with the more typical convention of -1 on error? Fixed in this file now.
+/* + * Create an iterator over the contents of the given variables. All variables + * must have entries in the hash table. + * The iterator that is created processes all given variables in parallel, + * meaning it will access $ITEM1[0] and $ITEM2[0] then $ITEM1[1] and $ITEM2[1] + * upto $ITEM1[n] and $ITEM2[n]. For this to work, the cardinality of all
s/upto/up to/ fixed all spelling mistakes
+typedef struct _virNWFilterVarCombIter virNWFilterVarCombIter; +typedef virNWFilterVarCombIter *virNWFilterVarCombIterPtr; +struct _virNWFilterVarCombIter { + virNWFilterHashTablePtr hashTable; + virNWFilterVarCombIterEntry iter[1];
1-element arrays look odd,
+ unsigned int nIter; +};
especially when they aren't the last element of a struct.
Yes, that was a misordering. Now it's also [0]. Stefan
This patch modifies the NWFilter parameter parser to support multiple elements with the same name and to internally build a list of items. An example of the XML looks like this: <parameter name='TEST' value='10.1.2.3'/> <parameter name='TEST' value='10.2.3.4'/> <parameter name='TEST' value='10.1.1.1'/> The list of values is then stored in the newly introduced data type virNWFilterVarValue. The XML formatter is also adapted to print out all items in alphabetical order sorted by 'name'. This patch als fixes a bug in the XML schema on the way. v4: - schema: removed the <optional> node in favor of the <zeroOrMore> node v3: - Follow Daniel Berrange's suggestion of parsing a list as shown above - Rewrote XML formatter to print out parameters in alphabetical order v2: - check that each item in the list only contains allowed characters Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/schemas/nwfilter.rng | 4 +- src/conf/nwfilter_params.c | 86 ++++++++++++++++++++++++++++----------------- 2 files changed, 56 insertions(+), 34 deletions(-) Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -674,7 +674,7 @@ isValidVarName(const char *var) static bool isValidVarValue(const char *value) { - return value[strspn(value, VALID_VARVALUE)] == 0; + return (value[strspn(value, VALID_VARVALUE)] == 0) && (strlen(value) != 0); } static virNWFilterVarValuePtr @@ -706,15 +706,22 @@ virNWFilterParseParamAttributes(xmlNodeP if (nam != NULL && val != NULL) { if (!isValidVarName(nam)) goto skip_entry; - value = virNWFilterParseVarValue(val); - if (!value) + if (!isValidVarValue(val)) goto skip_entry; - if (virNWFilterHashTablePut(table, nam, value, 1)) { - VIR_FREE(nam); - VIR_FREE(val); - virNWFilterVarValueFree(value); - virNWFilterHashTableFree(table); - return NULL; + value = virHashLookup(table->hashTable, nam); + if (value) { + /* add value to existing value -> list */ + if (!virNWFilterVarValueAddValue(value, val, false)) { + value = NULL; + goto err_exit; + } + val = NULL; + } else { + value = virNWFilterParseVarValue(val); + if (!value) + goto skip_entry; + if (virNWFilterHashTablePut(table, nam, value, 1)) + goto err_exit; } value = NULL; } @@ -727,40 +734,55 @@ skip_entry: cur = cur->next; } return table; -} - - -struct formatterParam { - virBufferPtr buf; - const char *indent; -}; +err_exit: + VIR_FREE(nam); + VIR_FREE(val); + virNWFilterVarValueFree(value); + virNWFilterHashTableFree(table); + return NULL; +} -static void -_formatParameterAttrs(void *payload, const void *name, void *data) +static int +virNWFilterFormatParameterNameSorter(const virHashKeyValuePairPtr a, + const virHashKeyValuePairPtr b) { - struct formatterParam *fp = (struct formatterParam *)data; - virNWFilterVarValuePtr value = payload; - - virBufferAsprintf(fp->buf, "%s<parameter name='%s' value='", - fp->indent, - (const char *)name); - virNWFilterVarValuePrint(value, fp->buf); - virBufferAddLit(fp->buf, "'/>\n"); + return strcmp((const char *)a->key, (const char *)b->key); } - char * virNWFilterFormatParamAttributes(virNWFilterHashTablePtr table, const char *indent) { virBuffer buf = VIR_BUFFER_INITIALIZER; - struct formatterParam fp = { - .buf = &buf, - .indent = indent, - }; + char **keys, *key; + int i, j, card, numKeys; + virNWFilterVarValuePtr value; + + if (!table) + return NULL; + + keys = (char **)virHashGetKeys(table->hashTable, + virNWFilterFormatParameterNameSorter); + if (!keys) + return NULL; + + numKeys = virHashSize(table->hashTable); + + for (i = 0; i < numKeys; i++) { + value = virHashLookup(table->hashTable, keys[i]); + card = virNWFilterVarValueGetCardinality(value); + + for (j = 0; j < card; j++) { + virBufferAsprintf(&buf, + "%s<parameter name='%s' value='%s'/>\n", + indent, keys[i], + virNWFilterVarValueGetNthValue(value, j)); + } + key = keys[i]; + } - virHashForEach(table->hashTable, _formatParameterAttrs, &fp); + virHashFreeKeys(table->hashTable, (void **)keys); if (virBufferError(&buf)) { virReportOOMError(); Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -312,7 +312,7 @@ <attribute name="filter"> <data type="NCName"/> </attribute> - <optional> + <zeroOrMore> <element name="parameter"> <attribute name="name"> <ref name="filter-param-name"/> @@ -321,7 +321,7 @@ <ref name="filter-param-value"/> </attribute> </element> - </optional> + </zeroOrMore> </define> <define name="rule-node-attributes">
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch modifies the NWFilter parameter parser to support multiple elements with the same name and to internally build a list of items. An example of the XML looks like this:
<parameter name='TEST' value='10.1.2.3'/> <parameter name='TEST' value='10.2.3.4'/> <parameter name='TEST' value='10.1.1.1'/>
Oh, I see - you fixed parsing to allow multiple, more or less replacing the part of patch 1/4 that was trying to do [a,b,c] formatting. We might as well ditch that part of the earlier patch, rather than introducing it just to pull it back out.
The list of values is then stored in the newly introduced data type virNWFilterVarValue.
The XML formatter is also adapted to print out all items in alphabetical order sorted by 'name'.
This patch als fixes a bug in the XML schema on the way.
s/als/also/
-static void -_formatParameterAttrs(void *payload, const void *name, void *data) +static int +virNWFilterFormatParameterNameSorter(const virHashKeyValuePairPtr a, + const virHashKeyValuePairPtr b) { - struct formatterParam *fp = (struct formatterParam *)data; - virNWFilterVarValuePtr value = payload; - - virBufferAsprintf(fp->buf, "%s<parameter name='%s' value='", - fp->indent, - (const char *)name); - virNWFilterVarValuePrint(value, fp->buf); - virBufferAddLit(fp->buf, "'/>\n"); + return strcmp((const char *)a->key, (const char *)b->key); }
- char * virNWFilterFormatParamAttributes(virNWFilterHashTablePtr table, const char *indent) { virBuffer buf = VIR_BUFFER_INITIALIZER; - struct formatterParam fp = { - .buf =&buf, - .indent = indent, - }; + char **keys, *key; + int i, j, card, numKeys; + virNWFilterVarValuePtr value; + + if (!table) + return NULL; + + keys = (char **)virHashGetKeys(table->hashTable, + virNWFilterFormatParameterNameSorter); + if (!keys) + return NULL; + + numKeys = virHashSize(table->hashTable); + + for (i = 0; i< numKeys; i++) { + value = virHashLookup(table->hashTable, keys[i]); + card = virNWFilterVarValueGetCardinality(value); + + for (j = 0; j< card; j++) { + virBufferAsprintf(&buf, + "%s<parameter name='%s' value='%s'/>\n", + indent, keys[i], + virNWFilterVarValueGetNthValue(value, j));
Are the parameter values guaranteed to be safe to print, or do you need virBufferEscapeString()? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/28/2011 05:38 PM, Eric Blake wrote:
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch modifies the NWFilter parameter parser to support multiple elements with the same name and to internally build a list of items. An example of the XML looks like this:
<parameter name='TEST' value='10.1.2.3'/> <parameter name='TEST' value='10.2.3.4'/> <parameter name='TEST' value='10.1.1.1'/>
Oh, I see - you fixed parsing to allow multiple, more or less replacing the part of patch 1/4 that was trying to do [a,b,c] formatting. We might as well ditch that part of the earlier patch, rather than introducing it just to pull it back out. Ditched now.
The list of values is then stored in the newly introduced data type virNWFilterVarValue.
The XML formatter is also adapted to print out all items in alphabetical order sorted by 'name'.
This patch als fixes a bug in the XML schema on the way.
s/als/also/
-static void -_formatParameterAttrs(void *payload, const void *name, void *data) +static int +virNWFilterFormatParameterNameSorter(const virHashKeyValuePairPtr a, + const virHashKeyValuePairPtr b) { - struct formatterParam *fp = (struct formatterParam *)data; - virNWFilterVarValuePtr value = payload; - - virBufferAsprintf(fp->buf, "%s<parameter name='%s' value='", - fp->indent, - (const char *)name); - virNWFilterVarValuePrint(value, fp->buf); - virBufferAddLit(fp->buf, "'/>\n"); + return strcmp((const char *)a->key, (const char *)b->key); }
- char * virNWFilterFormatParamAttributes(virNWFilterHashTablePtr table, const char *indent) { virBuffer buf = VIR_BUFFER_INITIALIZER; - struct formatterParam fp = { - .buf =&buf, - .indent = indent, - }; + char **keys, *key; + int i, j, card, numKeys; + virNWFilterVarValuePtr value; + + if (!table) + return NULL; + + keys = (char **)virHashGetKeys(table->hashTable, + virNWFilterFormatParameterNameSorter); + if (!keys) + return NULL; + + numKeys = virHashSize(table->hashTable); + + for (i = 0; i< numKeys; i++) { + value = virHashLookup(table->hashTable, keys[i]); + card = virNWFilterVarValueGetCardinality(value); + + for (j = 0; j< card; j++) { + virBufferAsprintf(&buf, + "%s<parameter name='%s' value='%s'/>\n", + indent, keys[i], + virNWFilterVarValueGetNthValue(value, j));
Are the parameter values guaranteed to be safe to print, or do you need virBufferEscapeString()?
Yes, since only these here are allowed: # define VALID_VARVALUE \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_.:" Stefan
This patch adds test cases for parsing of parameters with multiple occurrances of the same name. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- tests/nwfilterxml2xmlin/attr-value-test.xml | 23 +++++++++++++++++++++++ tests/nwfilterxml2xmlout/attr-value-test.xml | 18 ++++++++++++++++++ tests/nwfilterxml2xmltest.c | 2 ++ 3 files changed, 43 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml @@ -0,0 +1,23 @@ +<filter name='testcase'> + <uuid>83011800-f663-96d6-8841-fd836b4318c6</uuid> + <filterref filter='clean-traffic'> + <parameter name='a' value='1.2.3.4'/> + <parameter name='a' value='10.1.2.3'/> + <parameter name='a' value='10.3.3.3'/> + <parameter name='b' value='1.2.3.4'/> + </filterref> + <rule action='accept' direction='out'> + <mac srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + protocolid='arp'/> + </rule> + <rule action='accept' direction='out'> + <tcp srcmacaddr='1:2:3:4:5:6' + dstipaddr='10.1.2.3' dstipmask='255.255.255.255' + dscp='2'/> + </rule> + <rule action='accept' direction='out'> + <udp-ipv6 srcmacaddr='1:2:3:4:5:6' + dstipaddr='a:b:c::d:e:f' dstipmask='128' + dscp='2'/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmlout/attr-value-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/attr-value-test.xml @@ -0,0 +1,18 @@ +<filter name='testcase' chain='root'> + <uuid>83011800-f663-96d6-8841-fd836b4318c6</uuid> + <filterref filter='clean-traffic'> + <parameter name='a' value='1.2.3.4'/> + <parameter name='a' value='10.1.2.3'/> + <parameter name='a' value='10.3.3.3'/> + <parameter name='b' value='1.2.3.4'/> + </filterref> + <rule action='accept' direction='out' priority='500'> + <mac srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' protocolid='arp'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <tcp srcmacaddr='01:02:03:04:05:06' dstipaddr='10.1.2.3' dstipmask='32' dscp='2'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <udp-ipv6 srcmacaddr='01:02:03:04:05:06' dstipaddr='a:b:c::d:e:f' dstipmask='128' dscp='2'/> + </rule> +</filter> Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -150,6 +150,8 @@ mymain(void) DO_TEST("chain_prefixtest1", true); /* derived from arp-test */ + DO_TEST("attr-value-test", false); + return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); }
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch adds test cases for parsing of parameters with multiple occurrances of the same name.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- tests/nwfilterxml2xmlin/attr-value-test.xml | 23 +++++++++++++++++++++++ tests/nwfilterxml2xmlout/attr-value-test.xml | 18 ++++++++++++++++++ tests/nwfilterxml2xmltest.c | 2 ++ 3 files changed, 43 insertions(+)
Index: libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml @@ -0,0 +1,23 @@ +<filter name='testcase'> +<uuid>83011800-f663-96d6-8841-fd836b4318c6</uuid> +<filterref filter='clean-traffic'> +<parameter name='a' value='1.2.3.4'/> +<parameter name='a' value='10.1.2.3'/> +<parameter name='a' value='10.3.3.3'/> +<parameter name='b' value='1.2.3.4'/>
It's okay that the key (name) gets sorted when it appears multiple times, but we need to make sure the value doesn't get reordered. Can we mix it up just a bit more to make it obvious that no sorting on value is happening, such as: +<parameter name='a' value='1.2.3.4'/> +<parameter name='a' value='1.2.3.6'/> +<parameter name='a' value='1.2.3.5'/> Also, since we have a separate input from output, it might be worth intentionally putting name='b' out of order on the input, to show how it gets sorted into the output. But definitely a good idea to add tests. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/28/2011 05:46 PM, Eric Blake wrote:
On 10/27/2011 03:07 PM, Stefan Berger wrote:
This patch adds test cases for parsing of parameters with multiple occurrances of the same name.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- tests/nwfilterxml2xmlin/attr-value-test.xml | 23 +++++++++++++++++++++++ tests/nwfilterxml2xmlout/attr-value-test.xml | 18 ++++++++++++++++++ tests/nwfilterxml2xmltest.c | 2 ++ 3 files changed, 43 insertions(+)
Index: libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/attr-value-test.xml @@ -0,0 +1,23 @@ +<filter name='testcase'> +<uuid>83011800-f663-96d6-8841-fd836b4318c6</uuid> +<filterref filter='clean-traffic'> +<parameter name='a' value='1.2.3.4'/> +<parameter name='a' value='10.1.2.3'/> +<parameter name='a' value='10.3.3.3'/> +<parameter name='b' value='1.2.3.4'/>
It's okay that the key (name) gets sorted when it appears multiple times, but we need to make sure the value doesn't get reordered. Can we mix it up just a bit more to make it obvious that no sorting on value is happening, such as:
+<parameter name='a' value='1.2.3.4'/> +<parameter name='a' value='1.2.3.6'/> +<parameter name='a' value='1.2.3.5'/>
Also, since we have a separate input from output, it might be worth intentionally putting name='b' out of order on the input, to show how it gets sorted into the output.
But definitely a good idea to add tests.
I added some more tests here. Thanks for reviewing. I will send out v5 shortly. Stefan
participants (2)
-
Eric Blake -
Stefan Berger