[libvirt] [PATCH V4 1/4] Rework value part of name-value pairs
Eric Blake
eblake at redhat.com
Fri Oct 28 21:00:07 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list