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

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



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).

I'd accept it with the following changes merged in:

Cleanup some parts of the DHCP snooping v8 code addressing

- naming consistency
- memory leak
- if-before-free not being necessary
- separate shutdown function (for avoiding 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

 Regards,
    Stefan


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

Index: libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
===================================================================
--- libvirt-acl.orig/src/nwfilter/nwfilter_dhcpsnoop.c
+++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -75,15 +75,15 @@ static struct {
     { 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 +91,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 +105,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,9 +118,9 @@ virNWFilterSnoopIsActive(char *ThreadKey

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

@@ -740,7 +735,9 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
     virThread                   thread;

     virNWFilterSnoopIFKeyFMT(ifkey, vmuuid, macaddr);
+
     virNWFilterSnoopLock();
+
     req = virHashLookup(virNWFilterSnoopState.SnoopReqs, ifkey);
     isnewreq = req == NULL;
     if (!isnewreq) {
@@ -763,11 +760,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 err_exit;
     }

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

     req->vars = virNWFilterHashTableCreate(0);
     if (!req->vars) {
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
         virReportOOMError();
-        return -1;
+        goto err_exit;
     }
     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 err_exit;
     }
     req->driver = driver;

@@ -799,9 +791,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
_("virNWFilterDHCPSnoopReq ifname map failed" " on interface \"%s\" key \"%s\""), ifname,
                                ifkey);
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
-        return -1;
+        goto err_exit;
     }
     if (isnewreq &&
         virHashAddEntry(virNWFilterSnoopState.SnoopReqs, ifkey, req)) {
@@ -810,23 +800,27 @@ virNWFilterDHCPSnoopReq(virNWFilterTechD
                                " interface \"%s\" ifkey \"%s\""), ifname,
                                ifkey);
(void) virHashRemoveEntry(virNWFilterSnoopState.IfnameToKey, ifname);
-        virNWFilterSnoopUnlock();
-        virNWFilterSnoopReqFree(req);
-        return -1;
+        goto err_exit;
     }
-    virNWFilterSnoopUnlock();
+
     if (virThreadCreate(&thread, false, virNWFilterDHCPSnoop, req) != 0) {
-        virNWFilterSnoopLock();
(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 err_exit;
     }
+
+    virNWFilterSnoopUnlock();
+
     return 0;
+
+err_exit:
+    virNWFilterSnoopUnlock();
+    virNWFilterSnoopReqFree(req);
+
+    return -1;
 }

 /*
@@ -881,29 +875,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,10 +906,8 @@ 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);
@@ -924,8 +915,7 @@ virNWFilterDHCPSnoopEnd(const char *ifna
         if (!ifkey) {
             virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
_("ifname \"%s\" not in key map"), ifname);
-            virNWFilterSnoopUnlock();
-            return;
+            goto cleanup;
         }
     }

@@ -936,14 +926,12 @@ virNWFilterDHCPSnoopEnd(const char *ifna
         if (!req) {
             virNWFilterReportError(VIR_ERR_INTERNAL_ERROR,
                                    _("ifkey \"%s\" has no req"), ifkey);
-            virNWFilterSnoopUnlock();
-            return;
+            goto cleanup;
         }
         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);
@@ -952,23 +940,46 @@ virNWFilterDHCPSnoopEnd(const char *ifna
         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 the key / value pairs in the Active hash.
+     */
+
+    virNWFilterSnoopLock();
+
+    virNWFilterSnoopLeaseFileClose();
+    virHashFree(virNWFilterSnoopState.IfnameToKey);
+    virHashFree(virNWFilterSnoopState.SnoopReqs);
+
     virNWFilterSnoopUnlock();
+
+    virNWFilterSnoopActiveLock();
+
+    virHashFree(virNWFilterSnoopState.Active);
+
+    virNWFilterSnoopActiveUnlock();
 }

 static int
@@ -990,7 +1001,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));
@@ -1023,8 +1034,9 @@ 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) {
@@ -1192,6 +1204,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


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