4 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org