[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]