[libvirt] nwfilter: Enable detection of multiple IP addresses
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(-) Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -310,41 +310,97 @@ virNWFilterDeregisterLearnReq(int ifinde return res; } - - +/* Add an IP address to the list of IP addresses an interface is + * known to use. This function feeds the per-interface cache that + * is used to instantiate filters with variable '$IP'. + * + * @ifname: The name of the (tap) interface + * @addr: An IPv4 address in dotted decimal format that the (tap) + * interface is known to use. + * + * This function returns 0 on success, -1 otherwise + */ static int -virNWFilterAddIpAddrForIfname(const char *ifname, char *addr) { +virNWFilterAddIpAddrForIfname(const char *ifname, char *addr) +{ int ret; - virNWFilterVarValuePtr val = virNWFilterVarValueCreateSimple(addr); - - if (!val) - return 1; + virNWFilterVarValuePtr val; virMutexLock(&ipAddressMapLock); - ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1); + val = virHashLookup(ipAddressMap->hashTable, ifname); + if (!val) { + val = virNWFilterVarValueCreateSimple(addr); + if (!val) { + virReportOOMError(); + ret = -1; + goto err_exit; + } + ret = virNWFilterHashTablePut(ipAddressMap, ifname, val, 1); + } else { + if (virNWFilterVarValueAddValue(val, addr) < 0) + ret = -1; + } +err_exit: virMutexUnlock(&ipAddressMapLock); return ret; } #endif - -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 + */ +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; + virNWFilterVarValueDelValue(val, ipaddr); + ret = virNWFilterVarValueGetCardinality(val); + } + } else { +remove_entry: + /* remove whole entry */ + val = virNWFilterHashTableRemoveEntry(ipAddressMap, ifname); + if (val) { + ret = 0; + virNWFilterVarValueFree(val); + } + } virMutexUnlock(&ipAddressMapLock); -} + return ret; +} -const char * -virNWFilterGetIpAddrForIfname(const char *ifname) { +/* Get the list of IP addresses known to be in use by an interface + * + * This function returns NULL in case no IP address is known to be + * associated with the interface, a virNWFilterVarValuePtr otherwise + * that then can contain one or multiple entries. + */ +virNWFilterVarValuePtr +virNWFilterGetIpAddrForIfname(const char *ifname) +{ virNWFilterVarValuePtr res; virMutexLock(&ipAddressMapLock); @@ -353,10 +409,7 @@ virNWFilterGetIpAddrForIfname(const char virMutexUnlock(&ipAddressMapLock); - if (res) - return virNWFilterVarValueGetSimple(res); - - return NULL; + return res; } @@ -642,7 +695,10 @@ learnIPAddressThread(void *arg) char *inetaddr; if ((inetaddr = virSocketAddrFormat(&sa))!= NULL) { - virNWFilterAddIpAddrForIfname(req->ifname, inetaddr); + if (virNWFilterAddIpAddrForIfname(req->ifname, inetaddr) < 0) { + VIR_ERROR("Failed to add IP address %s to IP address cache " + "for interface %s", inetaddr, req->ifname); + } ret = virNWFilterInstantiateFilterLate(NULL, req->ifname, Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -846,9 +846,14 @@ virNWFilterVarCombIterCreate; virNWFilterVarCombIterFree; virNWFilterVarCombIterGetVarValue; virNWFilterVarCombIterNext; +virNWFilterVarValueAddValue; +virNWFilterVarValueCopy; virNWFilterVarValueCreateSimple; virNWFilterVarValueCreateSimpleCopyValue; +virNWFilterVarValueDelValue; +virNWFilterVarValueFree; virNWFilterVarValueGetSimple; +virNWFilterVarValueGetCardinality; # pci.h Index: libvirt-acl/src/conf/nwfilter_params.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.c +++ libvirt-acl/src/conf/nwfilter_params.c @@ -37,7 +37,7 @@ static bool isValidVarValue(const char *value); -static void +void virNWFilterVarValueFree(virNWFilterVarValuePtr val) { unsigned i; @@ -60,7 +60,7 @@ virNWFilterVarValueFree(virNWFilterVarVa VIR_FREE(val); } -static virNWFilterVarValuePtr +virNWFilterVarValuePtr virNWFilterVarValueCopy(const virNWFilterVarValuePtr val) { virNWFilterVarValuePtr res; @@ -222,6 +222,56 @@ virNWFilterVarValueAddValue(virNWFilterV return rc; } +static int +virNWFilterVarValueDelNthValue(virNWFilterVarValuePtr val, unsigned int pos) +{ + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + return -1; + + case NWFILTER_VALUE_TYPE_ARRAY: + if (pos < val->u.array.nValues) { + VIR_FREE(val->u.array.values[pos]); + val->u.array.nValues--; + + if (pos < val->u.array.nValues) + memmove(&val->u.array.values[pos], + &val->u.array.values[pos + 1], + sizeof(val->u.array.values[0]) * + (val->u.array.nValues - pos)); + return 0; + } + break; + + case NWFILTER_VALUE_TYPE_LAST: + break; + } + + return -1; +} + +int +virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value) +{ + unsigned int i; + + switch (val->valType) { + case NWFILTER_VALUE_TYPE_SIMPLE: + return -1; + + case NWFILTER_VALUE_TYPE_ARRAY: + for (i = 0; i < val->u.array.nValues; i++) + if (STREQ(value, val->u.array.values[i])) + return virNWFilterVarValueDelNthValue(val, i); + break; + + case NWFILTER_VALUE_TYPE_LAST: + break; + } + + return -1; +} + void virNWFilterVarCombIterFree(virNWFilterVarCombIterPtr ci) { @@ -521,14 +571,14 @@ virNWFilterHashTableCreate(int n) { } -int +void * virNWFilterHashTableRemoveEntry(virNWFilterHashTablePtr ht, const char *entry) { int i; - int rc = virHashRemoveEntry(ht->hashTable, entry); + void *value = virHashSteal(ht->hashTable, entry); - if (rc == 0) { + if (value) { for (i = 0; i < ht->nNames; i++) { if (STREQ(ht->names[i], entry)) { VIR_FREE(ht->names[i]); @@ -538,7 +588,7 @@ virNWFilterHashTableRemoveEntry(virNWFil } } } - return rc; + return value; } Index: libvirt-acl/src/conf/nwfilter_params.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_params.h +++ libvirt-acl/src/conf/nwfilter_params.h @@ -50,11 +50,14 @@ struct _virNWFilterVarValue { virNWFilterVarValuePtr virNWFilterVarValueCreateSimple(char *); virNWFilterVarValuePtr virNWFilterVarValueCreateSimpleCopyValue(const char *); +virNWFilterVarValuePtr virNWFilterVarValueCopy(const virNWFilterVarValuePtr); +void virNWFilterVarValueFree(virNWFilterVarValuePtr val); const char *virNWFilterVarValueGetSimple(const virNWFilterVarValuePtr val); const char *virNWFilterVarValueGetNthValue(virNWFilterVarValuePtr val, unsigned int idx); unsigned int virNWFilterVarValueGetCardinality(const virNWFilterVarValuePtr); int virNWFilterVarValueAddValue(virNWFilterVarValuePtr val, char *value); +int virNWFilterVarValueDelValue(virNWFilterVarValuePtr val, const char *value); typedef struct _virNWFilterHashTable virNWFilterHashTable; typedef virNWFilterHashTable *virNWFilterHashTablePtr; @@ -77,8 +80,8 @@ int virNWFilterHashTablePut(virNWFilterH const char *name, virNWFilterVarValuePtr val, int freeName); -int virNWFilterHashTableRemoveEntry(virNWFilterHashTablePtr table, - const char *name); +void *virNWFilterHashTableRemoveEntry(virNWFilterHashTablePtr table, + const char *name); int virNWFilterHashTablePutAll(virNWFilterHashTablePtr src, virNWFilterHashTablePtr dest); Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h @@ -61,7 +61,7 @@ int virNWFilterInstantiateFilterLate(vir int virNWFilterTeardownFilter(const virDomainNetDefPtr net); virNWFilterHashTablePtr virNWFilterCreateVarHashmap(char *macaddr, - char *ipaddr); + const virNWFilterVarValuePtr); void virNWFilterDomainFWUpdateCB(void *payload, const void *name, 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 @@ -145,7 +145,7 @@ virNWFilterRuleInstFree(virNWFilterRuleI static int virNWFilterVarHashmapAddStdValues(virNWFilterHashTablePtr table, char *macaddr, - char *ipaddr) + const virNWFilterVarValuePtr ipaddr) { virNWFilterVarValue *val; @@ -164,7 +164,7 @@ virNWFilterVarHashmapAddStdValues(virNWF } if (ipaddr) { - val = virNWFilterVarValueCreateSimple(ipaddr); + val = virNWFilterVarValueCopy(ipaddr); if (!val) return 1; @@ -194,7 +194,8 @@ virNWFilterVarHashmapAddStdValues(virNWF * is attached to the virConnect object. */ virNWFilterHashTablePtr -virNWFilterCreateVarHashmap(char *macaddr, char *ipaddr) { +virNWFilterCreateVarHashmap(char *macaddr, + const virNWFilterVarValuePtr ipaddr) { virNWFilterHashTablePtr table = virNWFilterHashTableCreate(0); if (!table) { virReportOOMError(); @@ -796,7 +797,7 @@ __virNWFilterInstantiateFilter(virConnec virNWFilterDefPtr filter; char vmmacaddr[VIR_MAC_STRING_BUFLEN] = {0}; char *str_macaddr = NULL; - const char *ipaddr; + virNWFilterVarValuePtr ipaddr; char *str_ipaddr = NULL; techdriver = virNWFilterTechDriverForName(drvname); @@ -836,16 +837,8 @@ __virNWFilterInstantiateFilter(virConnec } ipaddr = virNWFilterGetIpAddrForIfname(ifname); - if (ipaddr) { - str_ipaddr = strdup(ipaddr); - if (!str_ipaddr) { - virReportOOMError(); - rc = 1; - goto err_exit; - } - } - vars1 = virNWFilterCreateVarHashmap(str_macaddr, str_ipaddr); + vars1 = virNWFilterCreateVarHashmap(str_macaddr, ipaddr); if (!vars1) { rc = 1; goto err_exit; @@ -1101,7 +1094,7 @@ _virNWFilterTeardownFilter(const char *i techdriver->allTeardown(ifname); - virNWFilterDelIpAddrForIfname(ifname); + virNWFilterDelIpAddrForIfname(ifname, NULL); virNWFilterUnlockIface(ifname); Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.h +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h @@ -25,6 +25,8 @@ #ifndef __NWFILTER_LEARNIPADDR_H # define __NWFILTER_LEARNIPADDR_H +# include "conf/nwfilter_params.h" + enum howDetect { DETECT_DHCP = 1, DETECT_STATIC = 2, @@ -63,8 +65,8 @@ int virNWFilterLearnIPAddress(virNWFilte virNWFilterIPAddrLearnReqPtr virNWFilterLookupLearnReq(int ifindex); int virNWFilterTerminateLearnReq(const char *ifname); -void virNWFilterDelIpAddrForIfname(const char *ifname); -const char *virNWFilterGetIpAddrForIfname(const char *ifname); +int virNWFilterDelIpAddrForIfname(const char *ifname, const char *ipaddr); +virNWFilterVarValuePtr virNWFilterGetIpAddrForIfname(const char *ifname); int virNWFilterLockIface(const char *ifname) ATTRIBUTE_RETURN_CHECK; void virNWFilterUnlockIface(const char *ifname);
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
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.
participants (2)
-
Eric Blake -
Stefan Berger