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

On 03/30/2012 03:07 PM, David L Stevens wrote: 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