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 not in table NULL 0
+ */ +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...
+ 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 :) I think the logic bug fix warrants a v2, but the bulk of the patch looks good. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org