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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list