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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list