[libvirt] nwfilter: Enable detection of multiple IP addresses

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111122/19534ef2/attachment-0001.sig>