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

Re: [libvirt] [libvirt PATCHv5 2/2] add leasefile support



On 11/10/2011 03:28 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 |  270 +++++++++++++++++++++++++++++++++----
  1 files changed, 243 insertions(+), 27 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 8a37a6f..918ad7b 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -55,10 +55,18 @@
  #include "nwfilter_gentech_driver.h"
  #include "nwfilter_ebiptables_driver.h"
  #include "nwfilter_dhcpsnoop.h"
+#include "virfile.h"
+#include "configmake.h"

  #define VIR_FROM_THIS VIR_FROM_NWFILTER

  #ifdef HAVE_LIBPCAP
+
+#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;
@@ -76,6 +84,7 @@ struct virNWFilterSnoopReq {
      const char               *filtername;
      virNWFilterHashTablePtr   vars;
      virNWFilterDriverStatePtr driver;
+    bool                      running;
      /* start and end of lease list, ordered by lease time */
      struct iplease           *start;
      struct iplease           *end;
@@ -96,7 +105,15 @@ 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 virNWFilterSnoopReq *newreq(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_restore(struct virNWFilterSnoopReq *req);
+static void lease_refresh(void);

  /*
   * ipl_ladd - add an IP lease to a list
@@ -187,7 +204,7 @@ ipl_install(struct iplease *ipl)
   * ipl_add - create or update an IP lease
   */
  static void
-ipl_add(struct iplease *plnew)
+ipl_add(struct iplease *plnew, bool update_leasefile)
  {
      struct iplease *pl;
      struct virNWFilterSnoopReq *req = plnew->ipl_req;
@@ -195,6 +212,8 @@ ipl_add(struct iplease *plnew)
      pl = ipl_getbyip(req->start, plnew->ipl_ipaddr);
      if (pl) {
          ipl_update(pl, plnew->ipl_timeout);
+        if (update_leasefile)
+            lease_save(pl);
          return;
      }
      /* support for multiple addresses requires the ability to add filters
@@ -212,11 +231,14 @@ ipl_add(struct iplease *plnew)
      }
      *pl = *plnew;

-    if (ipl_install(pl)<  0) {
+    if (req->running&&  ipl_install(pl)<  0) {
          VIR_FREE(pl);
          return;
      }
      ipl_tadd(pl);
+    nleases++;
+    if (update_leasefile)
+        lease_save(pl);
  }

  /*
@@ -252,7 +274,7 @@ ipl_tdel(struct iplease *ipl)
   * ipl_del - delete an IP lease
   */
  static void
-ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr)
+ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr, bool update_leasefile)
  {
      struct iplease *ipl;

@@ -262,13 +284,18 @@ ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr)

      ipl_tdel(ipl);

-    /* for multiple address support, this needs to remove those rules
-     * referencing "IP" with ipl's ip value.
-     */
-    if (req->techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, NULL)) {
-        virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "ipl_ldel failed");
+    if (update_leasefile) {
+        lease_save(ipl);
+
+        /*
+         * for multiple address support, this needs to remove those rules
+         * referencing "IP" with ipl's ip value.
+         */
+        if (req->techdriver->applyDHCPOnlyRules(req->ifname,req->macaddr,NULL))
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "ipl_ldel failed");
      }
      VIR_FREE(ipl);
+    nleases--;
  }

  /*
@@ -308,7 +335,7 @@ ipl_trun(struct virNWFilterSnoopReq *req)

      now = time(0);
      while (req->start&&  req->start->ipl_timeout<= now)
-        ipl_del(req, req->start->ipl_ipaddr);
+        ipl_del(req, req->start->ipl_ipaddr, 1);
      return 0;
  }

@@ -467,11 +494,11 @@ dhcpdecode(struct virNWFilterSnoopReq *req, struct eth *pep, int len)

      switch (mtype) {
          case DHCPACK:
-            ipl_add(&ipl);
+            ipl_add(&ipl, 1);
              break;
          case DHCPDECLINE:
          case DHCPRELEASE:
-            ipl_del(req, ipl.ipl_ipaddr);
+            ipl_del(req, ipl.ipl_ipaddr, 1);
              break;
          default:
              break;
@@ -521,7 +548,7 @@ snoopReqFree(struct virNWFilterSnoopReq *req)
      /* free all leases */
      snoop_lock();
      for (ipl = req->start; ipl; ipl = req->start)
-        ipl_del(req, ipl->ipl_ipaddr);
+        ipl_del(req, ipl->ipl_ipaddr, 0);
      snoop_unlock();

      /* free all req data */
@@ -547,6 +574,8 @@ virNWFilterDHCPSnoop(void *req0)

      ifindex = if_nametoindex(req->ifname);

+    lease_restore(req);
+
      while (1) {
          if (req->die)
              break;
@@ -583,23 +612,22 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
  {
      struct virNWFilterSnoopReq *req;
      pthread_t                   thread;
+    bool                        isnewreq;

      snoop_lock();
      req = virHashLookup(SnoopReqs, ifname);
-    snoop_unlock();
-    if (req)
-        return 0;
-
-    if (VIR_ALLOC(req)<  0) {
-        virReportOOMError();
-        return 1;
-    }
-
-    req->ifname = strdup(ifname);
-    if (req->ifname == NULL) {
-        snoopReqFree(req);
-        virReportOOMError();
-        return 1;
+    isnewreq = req == NULL;
+    if (!isnewreq) {
+        if (req->running) {
+            snoop_unlock();
+            return 0;
+        }
+        snoop_unlock();
+    } else {
+        snoop_unlock();
+        req = newreq(ifname);
+        if (!req)
+            return 1;
      }

      req->techdriver = techdriver;
@@ -634,8 +662,10 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver,
      }
      req->driver = driver;

+    req->running = 1;
+
      snoop_lock();
-    if (virHashAddEntry(SnoopReqs, ifname, req)) {
+    if (isnewreq&&  virHashAddEntry(SnoopReqs, ifname, req)) {
          snoop_unlock();
          virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                 _("virNWFilterDHCPSnoopReq req add failed on "
@@ -669,6 +699,20 @@ freeReq(void *req0, const void *name ATTRIBUTE_UNUSED)
      req->die = 1;
  }

+static void
+lease_close(void)
+{
+    VIR_FORCE_CLOSE(lease_fd);
+}
+
+static void
+lease_open(void)
+{
+    lease_close();
+
+    lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644);
+}
+
  int
  virNWFilterDHCPSnoopInit(void)
  {
@@ -687,6 +731,8 @@ virNWFilterDHCPSnoopInit(void)
          virReportOOMError();
          return -1;
      }
+    lease_load();
+    lease_open();
      snoop_unlock();
      return 0;
  }
@@ -709,10 +755,180 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
              virReportOOMError();
              return;
          }
+        lease_load();
      }
+    lease_close();
      snoop_unlock();
  }

+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);
+    return 0;
+}
+
+static void
+lease_save(struct iplease *ipl)
+{
+    struct virNWFilterSnoopReq *req = ipl->ipl_req;
+
+    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 struct virNWFilterSnoopReq *
+newreq(const char *ifname)
+{
+    struct virNWFilterSnoopReq *req;
+
+    if (VIR_ALLOC(req)<  0) {
+        virReportOOMError();
+        return NULL;
+    }
+    req->ifname = strdup(ifname);
+
+    return req;
+}
+
+static void
+SaveSnoopReqIter(void *payload,
+                 const void *name ATTRIBUTE_UNUSED,
+                 void *data)
+{
+    struct virNWFilterSnoopReq *req = payload;
+    int tfd = (int)data;
+    struct iplease *ipl;
+
+    for (ipl = req->start; ipl; ipl = ipl->ipl_next)
+        (void) lease_write(tfd, req->ifname, ipl);
+}
+
+static void
+lease_refresh(void)
+{
+    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;
+    }
+    if (SnoopReqs)
+        virHashForEach(SnoopReqs, SaveSnoopReqIter, (void *)tfd);
To avoid the warnings pass '&tfd' and adjust the iterator accordingly.

nwfilter/nwfilter_dhcpsnoop.c: In function 'SaveSnoopReqIter':
nwfilter/nwfilter_dhcpsnoop.c:826:15: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
nwfilter/nwfilter_dhcpsnoop.c: In function 'lease_refresh':
nwfilter/nwfilter_dhcpsnoop.c:848:53: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
+    (void) close(tfd);
Use VIR_FORCE_CLOSE().
+    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
+lease_load(void)
+{
+    char line[256], ifname[16], ipstr[16], srvstr[16];
+    struct iplease ipl;
+    struct virNWFilterSnoopReq *req;
+    time_t now;
+    FILE *fp;
+    int ln = 0;
+
+    fp = fopen(LEASEFILE, "r");
+    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, "%u %16s %16s %16s",&ipl.ipl_timeout,
+                   ifname, ipstr, srvstr)<  4) {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("lease_load lease file line %d corrupt"),
+                                   ln);
+            break;;
+        }
+        if (ipl.ipl_timeout&&  ipl.ipl_timeout<  now)
+            continue;
+        req = virHashLookup(SnoopReqs, ifname);
+        if (!req) {
+            req = newreq(ifname);
+            if (!req)
+               break;
+            if (virHashAddEntry(SnoopReqs, ifname, req)) {
+                snoopReqFree(req);
+                virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                       _("lease_load req add failed on "
+                                       "interface \"%s\""), ifname);
+                continue;
+            }
+        }
+
+        if (inet_pton(AF_INET, ipstr,&ipl.ipl_ipaddr)<= 0) {
+            virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
+                                  _("line %d corrupt ipaddr \"%s\""),
+                                  ln, ipstr);
+            continue;
+        }
+        (void) inet_pton(AF_INET, srvstr,&ipl.ipl_server);
+        ipl.ipl_req = req;
+
+        if (ipl.ipl_timeout)
+            ipl_add(&ipl, 0);
+        else
+            ipl_del(req, ipl.ipl_ipaddr, 0);
+    }
+    if (fp != NULL)
+        (void) fclose(fp);
Use VIR_FORCE_FCLOSE(). No checking for fp != NULL necessary then.
+    lease_refresh();
+}
+
+static void
+lease_restore(struct virNWFilterSnoopReq *req)
+{
+    struct iplease *ipl;
+
+    for (ipl=req->start; ipl; ipl=ipl->ipl_next)
+        (void) ipl_install(ipl);
+}
+
  #else /* HAVE_LIBPCAP */
  int
  virNWFilterDHCPSnoopInit(void)
The code looks good now, but please fix the nits. I'll test it more later today.

When I start a VM that was previously running and thus still has a lease and restart libvirt while it is booting, libvirtd seems to automatically rebuilds the filters to the previous leased IP address. Can we invalidate the previous leases once new detection starts? Also, I would rather NOT use the name of the interface as a criterion to disable previous leases but the MAC address of the interface since the name of the interface can change (and another VM can now have 'vnet0'). I think the MAC address that an IP address is associated with should go into the lease file rather than the interface name. Would it be possible to change it in that direction?

   Stefan


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