[libvirt] [PATCH V14 3/5] nwfilter: move code for IP address map into separate file

[Stefan Berger]

On 06/01/2012 01:31 PM, Eric Blake wrote:
> On 05/25/2012 05:56 AM, Stefan Berger wrote:
>> The goal of this patch is to prepare for support for multiple IP
>> addresses per interface in the DHCP snooping code.
>>
>> Move the code for the IP address map that maps interface names to
>> IP addresses into their own file. Rename the functions on the way
>> but otherwise leave the code as-is. Initialize this new layer
>> separately before dependent layers (iplearning, dhcpsnooping)
>> and shut it down after them.
>>
>> ---
>>   src/Makefile.am                        |    4
>>   src/conf/nwfilter_ipaddrmap.c          |  167 +++++++++++++++++++++++++++++++++
>>   src/conf/nwfilter_ipaddrmap.h          |   37 +++++++
>>   src/libvirt_private.syms               |    8 +
>>   src/nwfilter/nwfilter_driver.c         |   11 +-
>>   src/nwfilter/nwfilter_gentech_driver.c |    5
>>   src/nwfilter/nwfilter_learnipaddr.c    |  126 ------------------------
>>   src/nwfilter/nwfilter_learnipaddr.h    |    3
>>   8 files changed, 229 insertions(+), 132 deletions(-)
>>
>
>> @@ -67,10 +68,12 @@ static int
>>   nwfilterDriverStartup(int privileged) {
>>       char *base = NULL;
>>
>> -    if (virNWFilterDHCPSnoopInit()<  0)
>> +    if (virNWFilterIPAddrMapInit()<  0)
>>           return -1;
>>       if (virNWFilterLearnInit()<  0)
>> -        return -1;
>> +        goto err_exit_ipaddrmapshutdown;
>> +    if (virNWFilterDHCPSnoopInit()<  0)
>> +        goto err_exit_learnshutdown;
> You know, if you would make the shutdown functions a no-op if they were
> not previously init'ed, then a single conf_init_err label would suffice
> for all cleanup, rather than having to have one label per cleanup.  But
> that's not a showstopper for this patch.
>
>> +
>> +#define VIR_FROM_THIS VIR_FROM_NWFILTER
>> +
>> +static virMutex ipAddressMapLock;
>> +static virNWFilterHashTablePtr ipAddressMap;
> Is it really appropriate to have a single map shared across all
> hypervisors?  I think we will eventually need a followup patch that

This map helps to determine what IP addresses have been detected on 
individual network interfaces. It is based on the assumption that the 
interface names are unique and therefore having multiple different 
hypervisors running on a system should not interfer with the entries in 
the map -- if this assumption does not hold, then there would have to 
more interface maps or each interface would have to be prefixed maybe 
with the UUID of the VM, i.e., <vmuuid>-<iface name>. Do we have such a 
case?


> creates a virIPAddrMap object, and allocates the object during each call
> to the init function, so that multiple hypervisors can manage identical
> IP addresses across independent virtualization technologies without
> colliding into the same map all by having each hypervisor driver call an
> init function for its own map.  But that can be a followup, I'm okay
> with this patch going in without changing it right now.

The IP addresses are not the keys, the interface names are the keys in 
this map. So multiple interfaces (on different VLANs) may have the same 
IP addresses as far as this map is concerned.

>
> ACK.
>
  Pushed.