Re: [libvirt] [PATCH V14 3/5] nwfilter: move code for IP address map into separate file
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.