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

[Stefan Berger]

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