[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V4 1/4] Rework value part of name-value pairs



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]