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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list