7:11 a.m.
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