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

Re: [libvirt] [libvirt PATCHv3 09/10] add leasefile support



On 10/12/2011 03:50 PM, David L Stevens wrote:
	This patch adds support for saving DHCP snooping leases to an on-disk
file and restoring saved leases that are still active on restart.

Signed-off-by: David L Stevens<dlstevens us ibm com>
---
  src/nwfilter/nwfilter_dhcpsnoop.c |  370 +++++++++++++++++++++++++++++++++++--
  1 files changed, 353 insertions(+), 17 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index f784a29..eedf550 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -56,10 +56,21 @@
  #include "nwfilter_gentech_driver.h"
  #include "nwfilter_ebiptables_driver.h"
  #include "nwfilter_dhcpsnoop.h"
+#include "configmake.h"

  #define VIR_FROM_THIS VIR_FROM_NWFILTER

+#define LEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.leases"
+#define TMPLEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.ltmp"
+static int lease_fd = -1;
+static int nleases = 0; /* number of active leases */
+static int wleases = 0; /* number of written leases */
+
  static virHashTablePtr SnoopReqs;
+static pthread_mutex_t SnoopLock;
+
+#define snoop_lock()    { pthread_mutex_lock(&SnoopLock); }
+#define snoop_unlock()  { pthread_mutex_unlock(&SnoopLock); }

  struct virNWFilterSnoopReq {
      virConnectPtr             conn;
@@ -90,7 +101,14 @@ struct iplease {

  static struct iplease *ipl_getbyip(struct iplease *start, uint32_t ipaddr);
  static void ipl_update(struct iplease *pl, uint32_t timeout);
-
+
+static struct iflease *getiflease(const char *ifname);
+static void lease_open(void);
+static void lease_close(void);
+static void lease_load(void);
+static void lease_save(struct iplease *ipl);
+static void lease_refresh(void);
+static void lease_restore(struct virNWFilterSnoopReq *req);

  /*
   * ipl_ladd - add an IP lease to a list
@@ -150,6 +168,7 @@ ipl_add(struct iplease *plnew)
          ipl_update(pl, plnew->ipl_timeout);
          return;
      }
+    nleases++;
      if (VIR_ALLOC(pl)<  0) {
          virReportOOMError();
          return;
@@ -184,6 +203,7 @@ ipl_add(struct iplease *plnew)
          return;
      }
      ipl_tadd(pl);
+    lease_save(pl);
  }

  /*
@@ -231,6 +251,7 @@ ipl_del(struct iplease *ipl)
      req = ipl->ipl_req;

      ipl_tdel(ipl);
+    lease_save(ipl);

      if (inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) {
          ipstr = strdup(ipbuf);
@@ -248,6 +269,7 @@ ipl_del(struct iplease *ipl)
                                 _("ipl_del inet_ntop " "failed (0x%08X)"),
                                 ipl->ipl_ipaddr);
      VIR_FREE(ipl);
+    nleases--;
  }

  /*
@@ -259,6 +281,7 @@ ipl_update(struct iplease *ipl, uint32_t timeout)
      ipl_tdel(ipl);
      ipl->ipl_timeout = timeout;
      ipl_tadd(ipl);
+    lease_save(ipl);
      return;
  }

@@ -275,8 +298,6 @@ ipl_getbyip(struct iplease *start, uint32_t ipaddr)
      return pl;
  }

-#define GRACE   5
-
  /*
   * ipl_trun - run the IP lease timeout list
   */
@@ -465,6 +486,19 @@ dhcpopen(const char *intf)
      return handle;
  }

+/*
+ * snoopReqFree - release a snoop Req
+ */
+static void
+snoopReqFree(struct virNWFilterSnoopReq *req)
+{
+    if (req->ifname)
+        VIR_FREE(req->ifname);
+    if (req->vars)
+        virNWFilterHashTableFree(req->vars);
+    VIR_FREE(req);
+}
+
  static void *
  virNWFilterDHCPSnoop(void *req0)
  {
@@ -479,12 +513,19 @@ virNWFilterDHCPSnoop(void *req0)
      if (!handle)
          return 0;

+    /* restore any saved leases for this interface */
+    snoop_lock();
+    lease_restore(req);
+    snoop_unlock();
+
      ifindex = if_nametoindex(req->ifname);

      while (1) {
          if (req->die)
              break;
+        snoop_lock();
          ipl_trun(req);
+        snoop_unlock();

          packet = (struct eth *) pcap_next(handle,&hdr);

@@ -494,16 +535,18 @@ virNWFilterDHCPSnoop(void *req0)
              continue;
          }

+        snoop_lock();
          dhcpdecode(req, packet, hdr.caplen);
+        snoop_unlock();
      }
+
+    snoop_lock();
      /* free all leases */
      for (ipl = req->start; ipl; ipl = req->start)
          ipl_del(ipl);
+    snoop_unlock();

-    /* free all req data */
-    VIR_FREE(req->ifname);
-    virNWFilterHashTableFree(req->vars);
-    VIR_FREE(req);
+    snoopReqFree(req);
      return 0;
  }

@@ -518,9 +561,12 @@ virNWFilterDHCPSnoopReq(virConnectPtr conn,
  {
      struct virNWFilterSnoopReq *req;

+    snoop_lock();
      req = virHashLookup(SnoopReqs, ifname);
-    if (req)
+    snoop_unlock();
+    if (req) {
          return 0;
+    }
      if (VIR_ALLOC(req)<  0) {
          virReportOOMError();
          return 1;
@@ -533,28 +579,30 @@ virNWFilterDHCPSnoopReq(virConnectPtr conn,
      req->ifname = strdup(ifname);
      req->vars = virNWFilterHashTableCreate(0);
      if (!req->vars) {
+        snoopReqFree(req);
Following the lookup into the hashtable above you cannot just free the request. I suppose you'd first have to remove it from the hash table -- or did this maybe happen in the lines in between? This seems more like fix to the previous patch? The snoopReqFree() should really be introduced in the previous patch...

          virReportOOMError();
          return 1;
      }
      if (virNWFilterHashTablePutAll(vars, req->vars)) {
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("virNWFilterDHCPSnoopReq: can't copy variables"
-                                " on if %s"), ifname);
+                               _("virNWFilterDHCPSnoopReq: can't copy "
+                                 "variables on if %s"), ifname);
+        snoopReqFree(req);
          return 1;
      }
      req->driver = driver;
      req->start = req->end = 0;

      if (virHashAddEntry(SnoopReqs, ifname, req)) {
-        VIR_FREE(req);
+        snoopReqFree(req);
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("virNWFilterDHCPSnoopReq req add failed on"
-                                "interface \"%s\""), ifname);
+                               _("virNWFilterDHCPSnoopReq req add "
+                                 "failed oninterface \"%s\""), ifname);
          return 1;
      }
      if (pthread_create(&req->thread, NULL, virNWFilterDHCPSnoop, req) != 0) {
          (void) virHashRemoveEntry(SnoopReqs, ifname);
-        VIR_FREE(req);
+        (void) snoopReqFree(req);
no need to void the result since it doesn't return anything
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                 _("virNWFilterDHCPSnoopReq pthread_create failed"
                                  " oninterface \"%s\""), ifname);
@@ -577,12 +625,41 @@ freeReq(void *req0, const void *name ATTRIBUTE_UNUSED)
      req->die = 1;
  }

+static void
+lease_close(void)
+{
+    (void) close(lease_fd);
+    lease_fd = -1;
+}
Replace with VIR_FORCE_CLOSE().

+
+static void
+lease_open(void)
+{
+    lease_close();
+
+    lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644);
+}
+
  int
  virNWFilterDHCPSnoopInit(void)
  {
      if (SnoopReqs)
          return 0;
Since locking is done below I'd test for SnoopReq with the snoop lock being held otherwise you could (theoretically) have two threads running into the code below, at least the code looks that way.
+
+    if (pthread_mutex_init(&SnoopLock, 0)<  0)
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("pthread_mutex_init: %s"),
+                               strerror(errno));
+    snoop_lock();
Move this lock to the top.
+
+    lease_load();
+
+    lease_open();
+
      SnoopReqs = virHashCreate(0, freeReq);
+
+    snoop_unlock();
+
      if (!SnoopReqs) {
          virReportOOMError();
          return -1;
@@ -595,8 +672,267 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
  {
      if (!SnoopReqs)
          return;
-    if (ifname)
-        virHashRemoveEntry(SnoopReqs, ifname);
-    else                        /* free all of them */
+    snoop_lock();
+    if (!ifname) {     /* free all of them */
          virHashFree(SnoopReqs);
+        lease_refresh();
+    } else
+        virHashRemoveEntry(SnoopReqs, ifname);
+    lease_close();
+    snoop_unlock();
+}
+
+
+/* lease file handling */
+
+struct iflease {
+    char           *ifl_ifname;
+    struct iplease *ifl_start;
+    struct iplease *ifl_end;
+    struct iflease *ifl_prev;
+    struct iflease *ifl_next;
+};
+
+struct iflease *leases;
+
+static int
+lease_write(int lfd, const char *ifname, struct iplease *ipl)
+{
+    char lbuf[256],ipstr[16],dhcpstr[16];
+
+    if (inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipstr, sizeof ipstr) == 0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("inet_ntop(0x%08X) failed"), ipl->ipl_ipaddr);
+        return -1;
+    }
+    if (inet_ntop(AF_INET,&ipl->ipl_server, dhcpstr, sizeof dhcpstr) == 0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("inet_ntop(0x%08X) failed"), ipl->ipl_server);
+        return -1;
+    }
+    /* time intf ip dhcpserver */
+    snprintf(lbuf, sizeof(lbuf), "%u %s %s %s\n", ipl->ipl_timeout,
+             ifname, ipstr, dhcpstr);
+    if (write(lfd, lbuf, strlen(lbuf))<  0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("lease file write failed: %s"),
+                               strerror(errno));
+        return -1;
+    }
+    (void) fsync(lfd);
What if this call fails?
+    return 0;
+}
+
+static void
+lease_save(struct iplease *ipl)
+{
+    struct virNWFilterSnoopReq *req = ipl->ipl_req;
+    struct iflease *ifl;
+
+    /* add to the lease file list */
+    ifl = getiflease(ipl->ipl_req->ifname);
+    if (ifl) {
+        struct iplease *ifipl = ipl_getbyip(ifl->ifl_start, ipl->ipl_ipaddr);
+
+        if (ifipl) {
+            if (ipl->ipl_timeout) {
+                ifipl->ipl_timeout = ipl->ipl_timeout;
+                ifipl->ipl_server = ipl->ipl_server;
+            } else {
+                ipl_ldel(ifipl,&ifl->ifl_start,&ifl->ifl_end);
+                VIR_FREE(ifipl);
+            }
+        } else if (!VIR_ALLOC(ifipl)) {
+            ifipl->ipl_ipaddr = ipl->ipl_ipaddr;
+            ifipl->ipl_server = ipl->ipl_server;
+            ifipl->ipl_timeout = ipl->ipl_timeout;
+            ipl_ladd(ifipl,&ifl->ifl_start,&ifl->ifl_end);
+        }
+    }
+    if (lease_fd<  0)
+       lease_open();
+    if (lease_write(lease_fd, req->ifname, ipl)<  0)
+       return;
+    /* keep dead leases at<  ~95% of file size */
+    if (++wleases>= nleases*20)
+        lease_load();   /* load&  refresh lease file */
+}
+
+static void
+lease_restore(struct virNWFilterSnoopReq *req)
+{
+    struct iflease *ifl;
+    struct iplease *ipl;
+
+    ifl = getiflease(req->ifname);
+    if (!ifl)
+        return;
+    for (ipl = ifl->ifl_start; ipl; ipl = ipl->ipl_next) {
+        ipl->ipl_req = req;
+        ipl_add(ipl);
+    }
+}
+
+static struct iflease *
+getiflease(const char *ifname)
+{
+    struct iflease *ifl;
+
+    for (ifl=leases; ifl; ifl=ifl->ifl_next)
+         if (strcmp(ifname, ifl->ifl_ifname) == 0)
+             return ifl;
+    if (VIR_ALLOC(ifl)) {
+        virReportOOMError();
+        return 0;
+    }
+    ifl->ifl_ifname = strdup(ifname);
+    ifl->ifl_next = leases;
+    leases = ifl;
+    return ifl;
+}
+
+static void
+lease_refresh(void)
+{
+    struct iflease *ifl;
+    struct iplease *ipl;
+    int tfd;
+
+    (void) unlink(TMPLEASEFILE);
+    /* lease file loaded, delete old one */
+    tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644);
+    if (tfd<  0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("open(\"%s\"): %s"),
+                               TMPLEASEFILE, strerror(errno));
+        return;
+    }
+    for (ifl=leases; ifl; ifl=ifl->ifl_next)
+        for (ipl = ifl->ifl_start; ipl; ipl = ipl->ipl_next)
+             if (lease_write(tfd, ifl->ifl_ifname, ipl)<  0)
+                 break;
In case of error, shouldn't it unlink the file and return  and error ?
+    (void) close(tfd);
VIR_CLOSE() and handle error case ?
+    if (rename(TMPLEASEFILE, LEASEFILE)<  0) {
+        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("rename(\"%s\", \"%s\"): %s"),
+                               TMPLEASEFILE, LEASEFILE, strerror(errno));
+        (void) unlink(TMPLEASEFILE);
+    }
+    wleases = 0;
+    lease_open();
+}
+
+static void
+LoadSnoopReqIter(void *payload,
+                 const void *name ATTRIBUTE_UNUSED,
+                 void *data ATTRIBUTE_UNUSED)
+{
+    struct virNWFilterSnoopReq *req = payload;
+    struct iplease *ipl, *ifipl;
+    struct iflease *ifl;
+
+    ifl = getiflease(req->ifname);
+    if (!ifl)
+        return;
+    for (ipl = req->start; ipl; ipl = ipl->ipl_next) {
+        ifipl = ipl_getbyip(ifl->ifl_start, ipl->ipl_ipaddr);
+        if (ifipl) {
+            if (ifipl->ipl_timeout<  ipl->ipl_timeout) {
+                ifipl->ipl_timeout = ipl->ipl_timeout;
+                ifipl->ipl_server = ipl->ipl_server;
+            }
+            continue;
+        }
+        if (VIR_ALLOC(ifipl)) {
+            virReportOOMError();
+            continue;
+        }
+        ifipl->ipl_ipaddr = ipl->ipl_ipaddr;
+        ifipl->ipl_server = ipl->ipl_server;
+        ifipl->ipl_timeout = ipl->ipl_timeout;
+        ipl_ladd(ifipl,&ifl->ifl_start,&ifl->ifl_end);
+    }
+}
+
+static void
+lease_load(void)
+{
+    char line[256], ifname[16], ipstr[16], srvstr[16];
+    uint32_t ipaddr, svaddr;
+    FILE *fp;
+    int ln = 0;
+    time_t timeout, now;
+    struct iflease *ifl;
+    struct iplease *ipl;
+
+    fp = fopen(LEASEFILE, "r");
Didn't find the fclose()..
+    time(&now);
+    while (fp&&  fgets(line, sizeof(line), fp)) {
+        if (line[strlen(line)-1] != '\n') {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("lease_load lease file line %d corrupt"),
+                                   ln);
+            break;
+        }
+        ln++;
+        if (sscanf(line, "%lu %16s %16s %16s", (unsigned long *)&timeout,
+                   ifname, ipstr, srvstr)<  4) {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("lease_load lease file line %d corrupt"),
+                                   ln);
+            break;;
+        }
+        if (timeout&&  timeout<  now)
+            continue;
+        ifl = getiflease(ifname);
+        if (!ifl)
+            break;
+
+        if (inet_pton(AF_INET, ipstr,&ipaddr)<= 0) {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                  _("line %d corrupt ipaddr \"%s\""),
+                                  ln, ipstr);
+            VIR_FREE(ipl);
+            continue;
+        }
+        (void) inet_pton(AF_INET, srvstr,&svaddr);
+
+        ipl = ipl_getbyip(ifl->ifl_start, ipaddr);
+        if (ipl) {
+            if (timeout&&  timeout<  ipl->ipl_timeout)
+                continue; /* out of order lease? skip. */
+            ipl->ipl_timeout = timeout;
+            ipl->ipl_server = svaddr;
+            continue;
+        }
+        if (!timeout)
+            continue; /* don't add new lease deletions */
+        if (VIR_ALLOC(ipl)) {
+            virReportOOMError();
+            break;
+        }
+        ipl->ipl_ipaddr = ipaddr;
+        ipl->ipl_server = svaddr;
+        ipl->ipl_timeout = timeout;
+        ipl_ladd(ipl,&ifl->ifl_start,&ifl->ifl_end);
+    }
+    /* also load any active leases from memory, in case lease writes may
+     * have failed.
+     */
+    if (SnoopReqs)
+        virHashForEach(SnoopReqs, LoadSnoopReqIter, 0);
+    /* remove any deleted leases */
+    for (ifl = leases; ifl; ifl = ifl->ifl_next) {
+        struct iplease *iplnext;
+
+        for (ipl = ifl->ifl_start; ipl; ipl = iplnext) {
+            iplnext = ipl->ipl_next;
+            if (ipl->ipl_timeout == 0) {
+               ipl_ldel(ipl,&ifl->ifl_start,&ifl->ifl_end);
+               VIR_FREE(ipl);
+            }
+        }
+    }
+
+    lease_refresh();
  }
For every lease I guess you'd have to store the VLAN ID as well since VMs on different VLANs can have the same IP address. I am not sure what effect this has on the whole code. Maybe your snooping would have to look at the MAC header and extract VLAN identifier(s) [nested!] as well and store them here. I wonder about the MAC address that an IP address is associated and whether that should be recorded as well and used for determining whether a lease has expired.

    Stefan


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