[libvirt] nwfilter: Enable detection of multiple IP addresses

[Stefan Berger]

On 11/22/2011 05:23 PM, Eric Blake wrote:
> On 11/22/2011 03:08 PM, Stefan Berger wrote:
>> In preparation of DHCP Snooping and the detection of multiple IP
>> addresses per interface:
>>
>> The hash table that is used to collect the detected IP address of an
>> interface can so far only handle one IP address per interface. With
>> this patch we extend this to allow it to handle a list of IP addresses.
>>
>> Above changes the returned variable type of virNWFilterGetIpAddrForIfname()
>> from char * to virNWFilterVarValuePtr; adapt all existing functions calling
>> this function.
>>
>> ---
>>   src/conf/nwfilter_params.c             |   62 ++++++++++++++++++--
>>   src/conf/nwfilter_params.h             |    7 +-
>>   src/libvirt_private.syms               |    5 +
>>   src/nwfilter/nwfilter_gentech_driver.c |   21 ++-----
>>   src/nwfilter/nwfilter_gentech_driver.h |    2
>>   src/nwfilter/nwfilter_learnipaddr.c    |   98 +++++++++++++++++++++++++--------
>>   src/nwfilter/nwfilter_learnipaddr.h    |    6 +-
>>   7 files changed, 155 insertions(+), 46 deletions(-)
>>
>> -
>> -void
>> -virNWFilterDelIpAddrForIfname(const char *ifname) {
>> +/* Delete all or a specific IP address from an interface.
>> + *
>> + * @ifname: The name of the (tap) interface
>> + * @addr: An IPv4 address in dotted decimal format that the (tap)
>> + *        interface is not using anymore; provide NULL to remove all IP
>> + *        addresses associated with the given interface
>> + *
>> + * This function returns the number of IP addresses that are still
>> + * known to be associated with this interface, in case of an error
>> + * -1 is returned. Error conditions are:
>> + * - no IP addresses is known to be associated with an interface
> When should we return 0 vs. -1?  There are several situations, with this
> being my guess for the most useful semantics:
>
>   ifname        ipaddr                                      return
> in table      non-NULL, and associated with ifname       length - 1
> in table      non-NULL, but not found in ifname's list   length
> in table      NULL                                       0
> not in table  non-NULL                                   -1
I think this is the most critical one among the failure conditions. In 
any case, the caller can be sure that the IP address is gone after this 
call.
> not in table  NULL                                       0
>
I adapted it to this one.

>> + */
>> +int
>> +virNWFilterDelIpAddrForIfname(const char *ifname, const char *ipaddr)
>> +{
>> +    int ret = -1;
>> +    virNWFilterVarValuePtr val = NULL;
>>
>>       virMutexLock(&ipAddressMapLock);
>>
>> -    if (virHashLookup(ipAddressMap->hashTable, ifname))
>> -        virNWFilterHashTableRemoveEntry(ipAddressMap, ifname);
>> +    if (ipaddr != NULL) {
>> +        val = virHashLookup(ipAddressMap->hashTable, ifname);
>> +        if (val) {
>> +            if (virNWFilterVarValueGetCardinality(val) == 1)
>> +                goto remove_entry;
> This says that if ifname has exactly one ipaddr associated, then remove
> that address, even if it does not match the ipaddr...
>
Ooops. Thanks. This would be fixed by:

+            if (virNWFilterVarValueGetCardinality(val) == 1 &&
+                STREQ(ipaddr,
+                      virNWFilterVarValueGetNthValue(val, 0)))

>> +            virNWFilterVarValueDelValue(val, ipaddr);
> that we had intended to be filtering on.  This logic needs to be fixed.
>
>> +            ret = virNWFilterVarValueGetCardinality(val);
>> +        }
>> +    } else {
>> +remove_entry:
>> +        /* remove whole entry */
>> +        val = virNWFilterHashTableRemoveEntry(ipAddressMap, ifname);
>> +        if (val) {
>> +            ret = 0;
>> +            virNWFilterVarValueFree(val);
>> +        }
>> +    }
>>
>> +++ libvirt-acl/src/libvirt_private.syms
>> @@ -846,9 +846,14 @@ virNWFilterVarCombIterCreate;
>>   virNWFilterVarCombIterFree;
>>   virNWFilterVarCombIterGetVarValue;
>>   virNWFilterVarCombIterNext;
>> +virNWFilterVarValueAddValue;
>> +virNWFilterVarValueCopy;
>>   virNWFilterVarValueCreateSimple;
>>   virNWFilterVarValueCreateSimpleCopyValue;
>> +virNWFilterVarValueDelValue;
>> +virNWFilterVarValueFree;
>>   virNWFilterVarValueGetSimple;
>> +virNWFilterVarValueGetCardinality;
> Those last two lines aren't sorted :)
Fixed.
> I think the logic bug fix warrants a v2, but the bulk of the patch looks
> good.
>