Re: [libvirt] [PATCH V4 1/4] Rework value part of name-value pairs
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
+++
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;
+}
[libvirt] [PATCH V4 1/4] Rework value part of name-value pairs
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 stef...@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)
+
