[libvirt] [libvirt PATCHv8 1/1] add DHCP snooping

Stefan Berger stefanb at linux.vnet.ibm.com
Mon Apr 9 19:45:03 UTC 2012


On 03/30/2012 03:07 PM, David L Stevens wrote:
> This patch adds DHCP snooping support to libvirt. The learning method for
> IP addresses is specified by setting the "ip_learning" variable to one of
> "any" [default] (existing IP learning code), "none" (static only addresses)
> or "dhcp" (DHCP snooping).

This is the 2nd version to patch on top of this v8. Some more testing 
revealed a locking problem that I now fixed.


Cleanup some parts of the DHCP snooping v8 code addressing

- naming consistency
- memory leaks
- if-before-free not being necessary
- separate shutdown function (for avoiding a valgrind reporting memory leak)
- a compilation error ("%d", strlen())
- pass NULL for a pointer rather than 0
- use common exits where possible
- make 'make syntax-check' pass
- due to a locking bug resulting in a deadlock reworked the locking and
   introduced a reference counter for the SnoopReq that must be held while
   the 'req' variable is accessed; it resulred in finer grained locking with
   the virNWFilterSnoopLock()


---
  po/POTFILES.in                    |    1
  src/nwfilter/nwfilter_dhcpsnoop.c |  360 
+++++++++++++++++++++++++-------------
  src/nwfilter/nwfilter_dhcpsnoop.h |    1
  src/nwfilter/nwfilter_driver.c    |    6
  4 files changed, 246 insertions(+), 122 deletions(-)

Index: libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_dhcpsnoop.c
+++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -59,6 +59,7 @@ static struct {
      int                  LeaseFD;
      int                  nLeases; /* number of active leases */
      int                  wLeases; /* number of written leases */
+    int                  nThreads; /* number of running threads */
  /* thread management */
      virHashTablePtr      SnoopReqs;
      virHashTablePtr      IfnameToKey;
@@ -67,23 +68,21 @@ static struct {
      virMutex             ActiveLock;
  } virNWFilterSnoopState = {
      .LeaseFD = -1,
-    .SnoopLock = { .lock=PTHREAD_MUTEX_INITIALIZER },
-    .ActiveLock = { .lock=PTHREAD_MUTEX_INITIALIZER },
  };

  #define virNWFilterSnoopLock() \
      { virMutexLock(&virNWFilterSnoopState.SnoopLock); }
  #define virNWFilterSnoopUnlock() \
      { virMutexUnlock(&virNWFilterSnoopState.SnoopLock); }
-#define virNWFilterSnoopLockActive() \
+#define virNWFilterSnoopActiveLock() \
      { virMutexLock(&virNWFilterSnoopState.ActiveLock); }
-#define virNWFilterSnoopUnlockActive() \
+#define virNWFilterSnoopActiveUnlock() \
      { virMutexUnlock(&virNWFilterSnoopState.ActiveLock); }

  static char *
  virNWFilterSnoopActivate(virThreadPtr thread)
  {
-    char            *key, *data;
+    char            *key;
      unsigned long    threadID = (unsigned long int)thread->thread;

      if (virAsprintf(&key, "0x%0*lX", (int)sizeof(threadID)*2, 
threadID) < 0) {
@@ -91,16 +90,11 @@ virNWFilterSnoopActivate(virThreadPtr th
          return NULL;
      }

-    virNWFilterSnoopLockActive();
-    data = strdup("1");
-    if (data == NULL) {
-        virReportOOMError();
-        VIR_FREE(key);
-    } else if (virHashAddEntry(virNWFilterSnoopState.Active, key, data)) {
+    virNWFilterSnoopActiveLock();
+    if (virHashAddEntry(virNWFilterSnoopState.Active, key, (void *)0x1)) {
          VIR_FREE(key);
-        VIR_FREE(data);
      }
-    virNWFilterSnoopUnlockActive();
+    virNWFilterSnoopActiveUnlock();
      return key;
  }

@@ -110,10 +104,10 @@ virNWFilterSnoopCancel(char **ThreadKey)
      if (*ThreadKey == NULL)
          return;

-    virNWFilterSnoopLockActive();
+    virNWFilterSnoopActiveLock();
      (void) virHashRemoveEntry(virNWFilterSnoopState.Active, *ThreadKey);
-    *ThreadKey = NULL;
-    virNWFilterSnoopUnlockActive();
+    VIR_FREE(*ThreadKey);
+    virNWFilterSnoopActiveUnlock();
  }

  static bool
@@ -123,15 +117,18 @@ virNWFilterSnoopIsActive(char *ThreadKey

      if (ThreadKey == NULL)
          return 0;
-    virNWFilterSnoopLockActive();
+
+    virNWFilterSnoopActiveLock();
      entry = virHashLookup(virNWFilterSnoopState.Active, ThreadKey);
-    virNWFilterSnoopUnlockActive();
+    virNWFilterSnoopActiveUnlock();
+
      return entry != NULL;
  }

  #define VIR_IFKEY_LEN   ((VIR_UUID_STRING_BUFLEN) + 
(VIR_MAC_STRING_BUFLEN))

  struct virNWFilterSnoopReq {
+    unsigned int                         refctr;
      virNWFilterTechDriverPtr             techdriver;
      const char                          *ifname;
      int                                  ifindex;
@@ -168,6 +165,7 @@ static void virNWFilterSnoopUpdateLease(
                                          time_t timeout);

  static struct virNWFilterSnoopReq *virNWFilterSnoopNewReq(const char 
*ifkey);
+static void virNWFilterSnoopReqRelease(void *req0, const void *name 
ATTRIBUTE_UNUSED);

  static void virNWFilterSnoopLeaseFileOpen(void);
  static void virNWFilterSnoopLeaseFileClose(void);
@@ -176,6 +174,56 @@ static void virNWFilterSnoopLeaseFileSav
  static void virNWFilterSnoopLeaseFileRestore(struct 
virNWFilterSnoopReq *req);
  static void virNWFilterSnoopLeaseFileRefresh(void);

+static void
+virNWFilterSnoopReqGet(struct virNWFilterSnoopReq *req)
+{
+    req->refctr++;
+}
+
+static struct virNWFilterSnoopReq *
+virNWFilterSnoopReqGetByKey(const char *ifkey)
+{
+    struct virNWFilterSnoopReq *req;
+
+    virNWFilterSnoopLock();
+
+    req = virHashLookup(virNWFilterSnoopState.SnoopReqs, ifkey);
+    if (req)
+        virNWFilterSnoopReqGet(req);
+
+    virNWFilterSnoopUnlock();
+
+    return req;
+}
+
+static void
+virNWFilterSnoopReqPut(struct virNWFilterSnoopReq *req)
+{
+    if (!req)
+        return;
+
+    virNWFilterSnoopLock()
+
+    req->refctr--;
+
+    if (req->refctr == 0) {
+        /*
+         * delete the request:
+         * - if we don't find it on the global list anymore
+         * we would keep the request:
+         * - if we still have a valid lease, keep the req for restarts
+         */
+        if (virHashLookup(virNWFilterSnoopState.SnoopReqs, req->ifkey) 
!= req) {
+            virNWFilterSnoopReqRelease(req, NULL);
+        } else if (!req->start || req->start->Timeout < time(0)) {
+            (void) virHashRemoveEntry(virNWFilterSnoopState.SnoopReqs,
+                                      req->ifkey);
+        }
+    }
+
+    virNWFilterSnoopUnlock();
+}
+
  /*
   * virNWFilterSnoopListAdd - add an IP lease to a list
   */
@@ -297,7 +345,11 @@ virNWFilterSnoopLeaseAdd(struct virNWFil
          return;
      }
      virNWFilterSnoopTimerAdd(pl);
+
+    virNWFilterSnoopLock();
      virNWFilterSnoopState.nLeases++;
+    virNWFilterSnoopUnlock();
+
      if (update_leasefile)
          virNWFilterSnoopLeaseFileSave(pl);
  }
@@ -361,7 +413,10 @@ virNWFilterSnoopLeaseDel(struct virNWFil
                                     _("virNWFilterSnoopListDel failed"));
      }
      VIR_FREE(ipl);
+
+    virNWFilterSnoopLock();
      virNWFilterSnoopState.nLeases--;
+    virNWFilterSnoopUnlock();
  }

  /*
@@ -624,6 +679,9 @@ virNWFilterSnoopReqFree(struct virNWFilt
      if (!req)
          return;

+    if (req->refctr != 0)
+        return;
+
      /* free all leases */
      for (ipl = req->start; ipl; ipl = req->start)
          virNWFilterSnoopLeaseDel(req, ipl->IPAddress, 0);
@@ -633,6 +691,7 @@ virNWFilterSnoopReqFree(struct virNWFilt
      VIR_FREE(req->linkdev);
      VIR_FREE(req->filtername);
      virNWFilterHashTableFree(req->vars);
+
      VIR_FREE(req);
  }

@@ -645,43 +704,38 @@ virNWFilterDHCPSnoop(void *req0)
      struct virNWFilterSnoopEthHdr  *packet;
      int                             ifindex;
      int                             errcount;
-    char                           *threadkey;
+    char                           *threadkey = NULL;
      virThread                       thread;

+    /* the thread starting us increased the reference counter for the 
req */
+
      handle = virNWFilterSnoopDHCPOpen(req->ifname);
      if (!handle)
-        return;
+        goto exit;

      virThreadSelf(&thread);
      req->threadkey = virNWFilterSnoopActivate(&thread);
      threadkey = strdup(req->threadkey);
      if (threadkey == NULL) {
          virReportOOMError();
-        pcap_close(handle);
-        return;
+        goto exit;
      }

      ifindex = if_nametoindex(req->ifname);

-    virNWFilterSnoopLock();
      virNWFilterSnoopLeaseFileRestore(req);
-    virNWFilterSnoopUnlock();

      errcount = 0;
      while (1) {
          int rv;

-        virNWFilterSnoopLock();
          virNWFilterSnoopTimerRun(req);
-        virNWFilterSnoopUnlock();

          rv = pcap_next_ex(handle, &hdr, (const u_char **)&packet);

-        if (!virNWFilterSnoopIsActive(threadkey)) {
-            VIR_FREE(threadkey);
-            pcap_close(handle);
-            return;
-        }
+        if (!virNWFilterSnoopIsActive(threadkey))
+            goto exit;
+
          if (rv < 0) {
              if (virNetDevValidateConfig(req->ifname, NULL, ifindex) <= 0)
                  break;
@@ -697,20 +751,31 @@ virNWFilterDHCPSnoop(void *req0)
              continue;
          }
          errcount = 0;
-        if (rv) {
-            virNWFilterSnoopLock();
+        if (rv)
              virNWFilterSnoopDHCPDecode(req, packet, hdr->caplen);
-            virNWFilterSnoopUnlock();
-        }
      }
      virNWFilterSnoopCancel(&req->threadkey);
+
+    virNWFilterSnoopLock();
+
      (void) virHashRemoveEntry(virNWFilterSnoopState.IfnameToKey, 
req->ifname);
+
+    virNWFilterSnoopUnlock();
+
      VIR_FREE(req->ifname);
-    /* if we still have a valid lease, keep the req for restarts */
-    if (!req->start || req->start->Timeout < time(0))
-        (void) virHashRemoveEntry(virNWFilterSnoopState.SnoopReqs, 
req->ifkey);
+
+exit:
+    virNWFilterSnoopReqPut(req);
+
      VIR_FREE(threadkey);
-    pcap_close(handle);
+
+    if (handle)
+        pcap_close(handle);
+
+    virNWFilterSnoopLock();
+    virNWFilterSnoopState.nThreads--;
+    virNWFilterSnoopUnlock();
+
      return;
  }

@@ -740,22 +805,23 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
      virThread                   thread;

      virNWFilterSnoopIFKeyFMT(ifkey, vmuuid, macaddr);
-    virNWFilterSnoopLock();
-    req = virHashLookup(virNWFilterSnoopState.SnoopReqs, ifkey);
-    isnewreq = req == NULL;
+
+    req = virNWFilterSnoopReqGetByKey(ifkey);
+    isnewreq = (req == NULL);
      if (!isnewreq) {
          if (req->threadkey) {
-            virNWFilterSnoopUnlock();
+            virNWFilterSnoopReqPut(req);
              return 0;
          }
      } else {
          req = virNWFilterSnoopNewReq(ifkey);
-        if (!req) {
-            virNWFilterSnoopUnlock();
+        if (!req)
              return -1;
-        }
+
+        virNWFilterSnoopReqGet(req);
      }

+    req->driver = driver;
      req->techdriver = techdriver;
      req->ifindex = if_nametoindex(ifname);
      req->linkdev = linkdev ? strdup(linkdev) : NULL;
@@ -763,11 +829,10 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
      req->ifname = strdup(ifname);
      memcpy(req->macaddr, macaddr, sizeof(req->macaddr));
      req->filtername = strdup(filtername);
+
      if (req->ifname == NULL || req->filtername == NULL) {
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
          virReportOOMError();
-        return -1;
+        goto exit_snoopreqput;
      }

      if (techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, NULL,
@@ -778,20 +843,18 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD

      req->vars = virNWFilterHashTableCreate(0);
      if (!req->vars) {
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
          virReportOOMError();
-        return -1;
+        goto exit_snoopreqput;
      }
+
      if (virNWFilterHashTablePutAll(filterparams, req->vars)) {
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                 _("virNWFilterDHCPSnoopReq: can't copy 
variables"
                                  " on if %s"), ifkey);
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
-        return -1;
+        goto exit_snoopreqput;
      }
-    req->driver = driver;
+
+    virNWFilterSnoopLock();

      if (virHashAddEntry(virNWFilterSnoopState.IfnameToKey, ifname,
                          req->ifkey)) {
@@ -799,9 +862,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
                                 _("virNWFilterDHCPSnoopReq ifname map 
failed"
                                   " on interface \"%s\" key \"%s\""), 
ifname,
                                 ifkey);
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
-        return -1;
+        goto exit_snoopunlock;
      }
      if (isnewreq &&
          virHashAddEntry(virNWFilterSnoopState.SnoopReqs, ifkey, req)) {
@@ -810,23 +871,30 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
                                 " interface \"%s\" ifkey \"%s\""), ifname,
                                 ifkey);
          (void) virHashRemoveEntry(virNWFilterSnoopState.IfnameToKey, 
ifname);
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
-        return -1;
+        goto exit_snoopunlock;
      }
-    virNWFilterSnoopUnlock();
+
+    virNWFilterSnoopState.nThreads++;
+
      if (virThreadCreate(&thread, false, virNWFilterDHCPSnoop, req) != 0) {
-        virNWFilterSnoopLock();
+        virNWFilterSnoopState.nThreads--;
          (void) virHashRemoveEntry(virNWFilterSnoopState.IfnameToKey, 
ifname);
-        (void) virHashRemoveEntry(virNWFilterSnoopState.SnoopReqs, ifkey);
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                 _("virNWFilterDHCPSnoopReq 
virThreadCreate "
                                   "failed on interface \"%s\""), ifname);
-        return -1;
+        goto exit_snoopunlock;
      }
+
+    virNWFilterSnoopUnlock();
+
      return 0;
+
+exit_snoopunlock:
+    virNWFilterSnoopUnlock();
+exit_snoopreqput:
+    virNWFilterSnoopReqPut(req);
+
+    return -1;
  }

  /*
@@ -842,6 +910,7 @@ virNWFilterSnoopReqRelease(void *req0, c

      if (req->threadkey)
          virNWFilterSnoopCancel(&req->threadkey);
+
      virNWFilterSnoopReqFree(req);
  }

@@ -863,13 +932,21 @@ virNWFilterSnoopLeaseFileOpen(void)
  int
  virNWFilterDHCPSnoopInit(void)
  {
-    if (virNWFilterSnoopState.SnoopReqs)
+    if (virNWFilterSnoopState.SnoopReqs) {
          return 0;
+    }
+
+    if (virMutexInitRecursive(&virNWFilterSnoopState.SnoopLock) < 0)
+        return -1;
+    if (virMutexInit(&virNWFilterSnoopState.ActiveLock) < 0)
+        return -1;

      virNWFilterSnoopLock();
+
      virNWFilterSnoopState.IfnameToKey = virHashCreate(0, NULL);
      virNWFilterSnoopState.SnoopReqs =
          virHashCreate(0, virNWFilterSnoopReqRelease);
+
      if (!virNWFilterSnoopState.IfnameToKey ||
          !virNWFilterSnoopState.SnoopReqs) {
          virNWFilterSnoopUnlock();
@@ -881,29 +958,28 @@ virNWFilterDHCPSnoopInit(void)

      virNWFilterSnoopUnlock();

-    virNWFilterSnoopLockActive();
-    virNWFilterSnoopState.Active = virHashCreate(0, 0);
+    virNWFilterSnoopActiveLock();
+
+    virNWFilterSnoopState.Active = virHashCreate(0, NULL);
      if (!virNWFilterSnoopState.Active) {
-        virNWFilterSnoopUnlockActive();
+        virNWFilterSnoopActiveUnlock();
          virReportOOMError();
          goto errexit;
      }
-    virNWFilterSnoopUnlockActive();
+    virNWFilterSnoopActiveUnlock();
+
      return 0;

  errexit:
-    if (virNWFilterSnoopState.IfnameToKey) {
-        virHashFree(virNWFilterSnoopState.IfnameToKey);
-        virNWFilterSnoopState.IfnameToKey = 0;
-    }
-    if (virNWFilterSnoopState.SnoopReqs) {
-        virHashFree(virNWFilterSnoopState.SnoopReqs);
-        virNWFilterSnoopState.SnoopReqs = 0;
-    }
-    if (virNWFilterSnoopState.Active) {
-        virHashFree(virNWFilterSnoopState.Active);
-        virNWFilterSnoopState.Active = 0;
-    }
+    virHashFree(virNWFilterSnoopState.IfnameToKey);
+    virNWFilterSnoopState.IfnameToKey = NULL;
+
+    virHashFree(virNWFilterSnoopState.SnoopReqs);
+    virNWFilterSnoopState.SnoopReqs = NULL;
+
+    virHashFree(virNWFilterSnoopState.Active);
+    virNWFilterSnoopState.Active = NULL;
+
      return -1;
  }

@@ -913,64 +989,87 @@ virNWFilterDHCPSnoopEnd(const char *ifna
      char *ifkey = NULL;

      virNWFilterSnoopLock();
-    if (!virNWFilterSnoopState.SnoopReqs) {
-        virNWFilterSnoopUnlock();
-        return;
-    }
+    if (!virNWFilterSnoopState.SnoopReqs)
+        goto cleanup;

      if (ifname) {
-        ifkey = (char 
*)virHashLookup(virNWFilterSnoopState.IfnameToKey,ifname);
+        ifkey = (char *)virHashLookup(virNWFilterSnoopState.IfnameToKey,
+                                      ifname);
          (void) virHashRemoveEntry(virNWFilterSnoopState.IfnameToKey, 
ifname);
          if (!ifkey) {
              virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                     _("ifname \"%s\" not in key map"), 
ifname);
-            virNWFilterSnoopUnlock();
-            return;
+            goto cleanup;
          }
      }

      if (ifkey) {
          struct virNWFilterSnoopReq *req;

-        req = virHashLookup(virNWFilterSnoopState.SnoopReqs, ifkey);
+        req = virNWFilterSnoopReqGetByKey(ifkey);
          if (!req) {
              virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                     _("ifkey \"%s\" has no req"), ifkey);
-            virNWFilterSnoopUnlock();
-            return;
-        }
-        if (!req->start || req->start->Timeout < time(0)) {
-            (void) virHashRemoveEntry(virNWFilterSnoopState.SnoopReqs,
-                                      req->ifkey);
-            virNWFilterSnoopUnlock();
-            return;
+            goto cleanup;
          }
          /* keep valid lease req; drop interface association */
          virNWFilterSnoopCancel(&req->threadkey);
+
          VIR_FREE(req->ifname);
+
+        virNWFilterSnoopReqPut(req);
      } else {                      /* free all of them */
          virNWFilterSnoopLeaseFileClose();
          virHashFree(virNWFilterSnoopState.IfnameToKey);
          virHashFree(virNWFilterSnoopState.SnoopReqs);
-        virNWFilterSnoopState.IfnameToKey = virHashCreate(0, 0);
+        virNWFilterSnoopState.IfnameToKey = virHashCreate(0, NULL);
          if (!virNWFilterSnoopState.IfnameToKey) {
-            virNWFilterSnoopUnlock();
              virReportOOMError();
-            return;
+            goto cleanup;
          }
          virNWFilterSnoopState.SnoopReqs =
              virHashCreate(0, virNWFilterSnoopReqRelease);
          if (!virNWFilterSnoopState.SnoopReqs) {
              virHashFree(virNWFilterSnoopState.IfnameToKey);
-            virNWFilterSnoopUnlock();
              virReportOOMError();
-            return;
+            goto cleanup;
          }
          virNWFilterSnoopLeaseFileLoad();
      }
+
+cleanup:
      virNWFilterSnoopUnlock();
  }

+void
+virNWFilterDHCPSnoopShutdown(void)
+{
+    /*
+     * free the SnoopReqs before the 'Active' hash since this will already
+     * clear some of the key / value pairs in the Active hash.
+     */
+
+    virNWFilterSnoopLock();
+
+    virNWFilterSnoopLeaseFileClose();
+    virHashFree(virNWFilterSnoopState.IfnameToKey);
+    virHashFree(virNWFilterSnoopState.SnoopReqs);
+
+    virNWFilterSnoopUnlock();
+
+    virNWFilterSnoopActiveLock();
+    virHashFree(virNWFilterSnoopState.Active);
+    virNWFilterSnoopActiveUnlock();
+
+    while (1) {
+        if (virNWFilterSnoopState.nThreads == 0)
+            break;
+
+        VIR_WARN("Waiting for snooping threads to terminate\n");
+        usleep(1000 * 1000);
+    }
+}
+
  static int
  virNWFilterSnoopLeaseFileWrite(int lfd, const char *ifkey,
                                 struct virNWFilterSnoopIPLease *ipl)
@@ -990,7 +1089,7 @@ virNWFilterSnoopLeaseFileWrite(int lfd,
      /* time intf ip dhcpserver */
      snprintf(lbuf, sizeof(lbuf), "%u %s %s %s\n", ipl->Timeout,
               ifkey, ipstr, dhcpstr);
-    if (write(lfd, lbuf, strlen(lbuf)) < 0) {
+    if (safewrite(lfd, lbuf, strlen(lbuf)) != strlen(lbuf)) {
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                 _("lease file write failed: %s"),
                                 strerror(errno));
@@ -1005,14 +1104,20 @@ virNWFilterSnoopLeaseFileSave(struct vir
  {
      struct virNWFilterSnoopReq *req = ipl->SnoopReq;

+    virNWFilterSnoopLock();
+
      if (virNWFilterSnoopState.LeaseFD < 0)
-       virNWFilterSnoopLeaseFileOpen();
+        virNWFilterSnoopLeaseFileOpen();
      if (virNWFilterSnoopLeaseFileWrite(virNWFilterSnoopState.LeaseFD,
-                                       req->ifkey, ipl) < 0)
-       return;
+                                       req->ifkey, ipl) < 0) {
+        virNWFilterSnoopUnlock();
+        return;
+    }
      /* keep dead leases at < ~95% of file size */
-    if (++virNWFilterSnoopState.wLeases >= 
virNWFilterSnoopState.nLeases*20)
+    if (++virNWFilterSnoopState.wLeases >= 
virNWFilterSnoopState.nLeases * 20)
          virNWFilterSnoopLeaseFileLoad();   /* load & refresh lease file */
+
+    virNWFilterSnoopUnlock();
  }

  static struct virNWFilterSnoopReq *
@@ -1023,14 +1128,17 @@ virNWFilterSnoopNewReq(const char *ifkey
      if (ifkey == NULL || strlen(ifkey) != VIR_IFKEY_LEN-1) {
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                 _("virNWFilterSnoopNewReq called with 
invalid "
-                                 "key \"%s\" (%d)"),
-                               ifkey ? ifkey : "", strlen(ifkey));
+                                 "key \"%s\" (%u)"),
+                               ifkey ? ifkey : "",
+                               (unsigned int)strlen(ifkey));
          return NULL;
      }
+
      if (VIR_ALLOC(req) < 0) {
          virReportOOMError();
          return NULL;
      }
+
      if (virStrcpyStatic(req->ifkey, ifkey) == NULL)
          VIR_FREE(req);

@@ -1077,6 +1185,9 @@ virNWFilterSnoopLeaseFileRefresh(void)
                                 TMPLEASEFILE, strerror(errno));
          return;
      }
+
+    virNWFilterSnoopLock();
+
      if (virNWFilterSnoopState.SnoopReqs)
          virHashForEach(virNWFilterSnoopState.SnoopReqs,
                         virNWFilterSnoopSaveIter, (void *)&tfd);
@@ -1089,6 +1200,8 @@ virNWFilterSnoopLeaseFileRefresh(void)
      }
      virNWFilterSnoopState.wLeases = 0;
      virNWFilterSnoopLeaseFileOpen();
+
+    virNWFilterSnoopUnlock();
  }


@@ -1101,7 +1214,7 @@ virNWFilterSnoopLeaseFileLoad(void)
      struct virNWFilterSnoopReq *req;
      time_t now;
      FILE *fp;
-    int ln = 0;
+    int ln = 0, tmp;

      fp = fopen(LEASEFILE, "r");
      time(&now);
@@ -1123,13 +1236,20 @@ virNWFilterSnoopLeaseFileLoad(void)
          }
          if (ipl.Timeout && ipl.Timeout < now)
              continue;
-        req = virHashLookup(virNWFilterSnoopState.SnoopReqs, ifkey);
+        req = virNWFilterSnoopReqGetByKey(ifkey);
          if (!req) {
              req = virNWFilterSnoopNewReq(ifkey);
              if (!req)
                 break;
-            if (virHashAddEntry(virNWFilterSnoopState.SnoopReqs, ifkey, 
req)) {
-                virNWFilterSnoopReqFree(req);
+
+            virNWFilterSnoopReqGet(req);
+
+            virNWFilterSnoopLock();
+            tmp = virHashAddEntry(virNWFilterSnoopState.SnoopReqs, 
ifkey, req);
+            virNWFilterSnoopUnlock();
+
+            if (tmp) {
+                virNWFilterSnoopReqPut(req);
                  virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                         
_("virNWFilterSnoopLeaseFileLoad req add"
                                           " failed on interface 
\"%s\""), ifkey);
@@ -1141,6 +1261,7 @@ virNWFilterSnoopLeaseFileLoad(void)
              virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                    _("line %d corrupt ipaddr \"%s\""),
                                    ln, ipstr);
+            virNWFilterSnoopReqPut(req);
              continue;
          }
          (void) inet_pton(AF_INET, srvstr, &ipl.IPServer);
@@ -1150,10 +1271,13 @@ virNWFilterSnoopLeaseFileLoad(void)
              virNWFilterSnoopLeaseAdd(&ipl, 0);
          else
              virNWFilterSnoopLeaseDel(req, ipl.IPAddress, 0);
+
+        virNWFilterSnoopReqPut(req);
      }
      if (fp != NULL)
          (void) fclose(fp);
      virNWFilterSnoopLeaseFileRefresh();
+
  }

  static void
@@ -1192,6 +1316,6 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
      virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("libvirt was not 
compiled "
                             "with libpcap and \"ip_learning='dhcp'\" 
requires"
                             " it."));
-    return 1;
+    return -1;
  }
  #endif /* HAVE_LIBPCAP */
Index: libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.h
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_dhcpsnoop.h
+++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.h
@@ -25,6 +25,7 @@
  #define __NWFILTER_DHCPSNOOP_H

  int virNWFilterDHCPSnoopInit(void);
+void virNWFilterDHCPSnoopShutdown(void);
  int virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
                              const char *ifname,
                              const char *linkdev,
Index: libvirt-acl/src/nwfilter/nwfilter_driver.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_driver.c
+++ libvirt-acl/src/nwfilter/nwfilter_driver.c
@@ -130,7 +130,7 @@ alloc_err_exit:

  conf_init_err:
      virNWFilterTechDriversShutdown();
-    virNWFilterDHCPSnoopEnd(0);
+    virNWFilterDHCPSnoopShutdown();
      virNWFilterLearnShutdown();

      return -1;
@@ -153,7 +153,7 @@ nwfilterDriverReload(void) {
      conn = virConnectOpen("qemu:///system");

      if (conn) {
-        virNWFilterDHCPSnoopEnd(0);
+        virNWFilterDHCPSnoopEnd(NULL);
          /* shut down all threads -- they will be restarted if necessary */
          virNWFilterLearnThreadsTerminate(true);

@@ -208,7 +208,7 @@ nwfilterDriverShutdown(void) {

      virNWFilterConfLayerShutdown();
      virNWFilterTechDriversShutdown();
-    virNWFilterDHCPSnoopEnd(0);
+    virNWFilterDHCPSnoopShutdown();
      virNWFilterLearnShutdown();

      nwfilterDriverLock(driverState);
Index: libvirt-acl/po/POTFILES.in
===================================================================
--- libvirt-acl.orig/po/POTFILES.in
+++ libvirt-acl/po/POTFILES.in
@@ -52,7 +52,6 @@ src/node_device/node_device_hal.c
  src/node_device/node_device_linux_sysfs.c
  src/node_device/node_device_udev.c
  src/nodeinfo.c
-src/nwfilter/nwfilter_dhcpsnoop.c
  src/nwfilter/nwfilter_driver.c
  src/nwfilter/nwfilter_ebiptables_driver.c
  src/nwfilter/nwfilter_gentech_driver.c




More information about the libvir-list mailing list