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