[libvirt PATCH 07/12] nwfilter_dhcpsnoop: Replace virNWFilterSnoopReqLock functions
Tim Wiederhake
twiederh at redhat.com
Wed Mar 9 11:02:25 UTC 2022
Use automatic mutex management instead.
Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
---
src/nwfilter/nwfilter_dhcpsnoop.c | 273 +++++++++++-------------------
1 file changed, 95 insertions(+), 178 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index f18f7b80d6..1cbb840749 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -139,7 +139,7 @@ struct _virNWFilterSnoopReq {
/*
* Note about lock-order:
* 1st: virNWFilterSnoopState.snoopLock
- * 2nd: virNWFilterSnoopReqLock(req)
+ * 2nd: &req->lock
*
* Rationale: Former protects the SnoopReqs hash, latter its contents
*/
@@ -246,9 +246,6 @@ static int virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req,
bool update_leasefile,
bool instantiate);
-static void virNWFilterSnoopReqLock(virNWFilterSnoopReq *req);
-static void virNWFilterSnoopReqUnlock(virNWFilterSnoopReq *req);
-
static void virNWFilterSnoopLeaseFileLoad(void);
static void virNWFilterSnoopLeaseFileSave(virNWFilterSnoopIPLease *ipl);
@@ -362,11 +359,9 @@ virNWFilterSnoopIPLeaseTimerAdd(virNWFilterSnoopIPLease *plnew)
virNWFilterSnoopReq *req = plnew->snoopReq;
/* protect req->start / req->end */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
virNWFilterSnoopListAdd(plnew, &req->start, &req->end);
-
- virNWFilterSnoopReqUnlock(req);
}
/*
@@ -378,12 +373,9 @@ virNWFilterSnoopIPLeaseTimerDel(virNWFilterSnoopIPLease *ipl)
virNWFilterSnoopReq *req = ipl->snoopReq;
/* protect req->start / req->end */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
virNWFilterSnoopListDel(ipl, &req->start, &req->end);
-
- virNWFilterSnoopReqUnlock(req);
-
ipl->timeout = 0;
}
@@ -401,8 +393,8 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLease *ipl,
bool instantiate)
{
g_autofree char *ipaddr = virSocketAddrFormat(&ipl->ipAddress);
- int rc = -1;
virNWFilterSnoopReq *req;
+ VIR_LOCK_GUARD lock = { NULL };
if (!ipaddr)
return -1;
@@ -410,27 +402,20 @@ virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLease *ipl,
req = ipl->snoopReq;
/* protect req->binding->portdevname */
- virNWFilterSnoopReqLock(req);
+ lock = virLockGuardLock(&req->lock);
if (virNWFilterIPAddrMapAddIPAddr(req->binding->portdevname, ipaddr) < 0)
- goto cleanup;
+ return -1;
- if (!instantiate) {
- rc = 0;
- goto cleanup;
- }
+ if (!instantiate)
+ return 0;
/* instantiate the filters */
- if (req->binding->portdevname) {
- rc = virNWFilterInstantiateFilterLate(req->driver,
- req->binding,
- req->ifindex);
- }
+ if (!req->binding->portdevname)
+ return -1;
- cleanup:
- virNWFilterSnoopReqUnlock(req);
- return rc;
+ return virNWFilterInstantiateFilterLate(req->driver, req->binding, req->ifindex);
}
/*
@@ -473,7 +458,7 @@ virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req)
bool is_last = false;
/* protect req->start */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
while (req->start && req->start->timeout <= now) {
if (req->start->next == NULL ||
@@ -483,8 +468,6 @@ virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReq *req)
is_last);
}
- virNWFilterSnoopReqUnlock(req);
-
return 0;
}
@@ -562,24 +545,6 @@ virNWFilterSnoopReqFree(virNWFilterSnoopReq *req)
g_free(req);
}
-/*
- * Lock a Snoop request 'req'
- */
-static void
-virNWFilterSnoopReqLock(virNWFilterSnoopReq *req)
-{
- virMutexLock(&req->lock);
-}
-
-/*
- * Unlock a Snoop request 'req'
- */
-static void
-virNWFilterSnoopReqUnlock(virNWFilterSnoopReq *req)
-{
- virMutexUnlock(&req->lock);
-}
-
/*
* virNWFilterSnoopReqRelease - hash table free function to kill a request
*/
@@ -592,12 +557,10 @@ virNWFilterSnoopReqRelease(void *req0)
return;
/* protect req->threadkey */
- virNWFilterSnoopReqLock(req);
-
- if (req->threadkey)
- virNWFilterSnoopCancel(&req->threadkey);
-
- virNWFilterSnoopReqUnlock(req);
+ VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
+ if (req->threadkey)
+ virNWFilterSnoopCancel(&req->threadkey);
+ }
virNWFilterSnoopReqFree(req);
}
@@ -658,45 +621,33 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReq *req,
virNWFilterSnoopIPLease *plnew,
bool update_leasefile)
{
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
virNWFilterSnoopIPLease *pl;
plnew->snoopReq = req;
- /* protect req->start and the lease */
- virNWFilterSnoopReqLock(req);
-
pl = virNWFilterSnoopIPLeaseGetByIP(req->start, &plnew->ipAddress);
if (pl) {
virNWFilterSnoopIPLeaseUpdate(pl, plnew->timeout);
+ virLockGuardUnlock(&lock);
+ } else {
+ pl = g_new0(virNWFilterSnoopIPLease, 1);
+ *pl = *plnew;
- virNWFilterSnoopReqUnlock(req);
-
- goto cleanup;
- }
-
- virNWFilterSnoopReqUnlock(req);
+ if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) {
+ g_free(pl);
+ return -1;
+ }
- pl = g_new0(virNWFilterSnoopIPLease, 1);
- *pl = *plnew;
+ virLockGuardUnlock(&lock);
- /* protect req->threadkey */
- virNWFilterSnoopReqLock(req);
+ /* put the lease on the req's list */
+ virNWFilterSnoopIPLeaseTimerAdd(pl);
- if (req->threadkey && virNWFilterSnoopIPLeaseInstallRule(pl, true) < 0) {
- virNWFilterSnoopReqUnlock(req);
- g_free(pl);
- return -1;
+ g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1);
}
- virNWFilterSnoopReqUnlock(req);
-
- /* put the lease on the req's list */
- virNWFilterSnoopIPLeaseTimerAdd(pl);
-
- g_atomic_int_add(&virNWFilterSnoopState.nLeases, 1);
-
- cleanup:
if (update_leasefile)
virNWFilterSnoopLeaseFileSave(pl);
@@ -710,24 +661,19 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterSnoopReq *req,
static int
virNWFilterSnoopReqRestore(virNWFilterSnoopReq *req)
{
- int ret = 0;
virNWFilterSnoopIPLease *ipl;
/* protect req->start */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
for (ipl = req->start; ipl; ipl = ipl->next) {
/* instantiate the rules at the last lease */
bool is_last = (ipl->next == NULL);
- if (virNWFilterSnoopIPLeaseInstallRule(ipl, is_last) < 0) {
- ret = -1;
- break;
- }
+ if (virNWFilterSnoopIPLeaseInstallRule(ipl, is_last) < 0)
+ return -1;
}
- virNWFilterSnoopReqUnlock(req);
-
- return ret;
+ return 0;
}
/*
@@ -759,16 +705,15 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req,
g_autofree char *ipstr = NULL;
/* protect req->start, req->ifname and the lease */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
ipl = virNWFilterSnoopIPLeaseGetByIP(req->start, ipaddr);
if (ipl == NULL)
- goto lease_not_found;
+ return 0;
ipstr = virSocketAddrFormat(&ipl->ipAddress);
if (!ipstr) {
- ret = -1;
- goto lease_not_found;
+ return -1;
}
virNWFilterSnoopIPLeaseTimerDel(ipl);
@@ -807,10 +752,6 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReq *req,
g_free(ipl);
ignore_value(!!g_atomic_int_dec_and_test(&virNWFilterSnoopState.nLeases));
-
- lease_not_found:
- virNWFilterSnoopReqUnlock(req);
-
return ret;
}
@@ -1279,42 +1220,36 @@ virNWFilterDHCPSnoopThread(void *req0)
/* whoever started us increased the reference counter for the req for us */
/* protect req->binding->portdevname & req->threadkey */
- virNWFilterSnoopReqLock(req);
-
- if (req->binding->portdevname && req->threadkey) {
- for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) {
- pcapConf[i].handle =
- virNWFilterSnoopDHCPOpen(req->binding->portdevname,
- &req->binding->mac,
- pcapConf[i].filter,
- pcapConf[i].dir);
- if (!pcapConf[i].handle) {
- error = true;
- break;
+ VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
+ if (req->binding->portdevname && req->threadkey) {
+ for (i = 0; i < G_N_ELEMENTS(pcapConf); i++) {
+ pcapConf[i].handle =
+ virNWFilterSnoopDHCPOpen(req->binding->portdevname,
+ &req->binding->mac,
+ pcapConf[i].filter,
+ pcapConf[i].dir);
+ if (!pcapConf[i].handle) {
+ error = true;
+ break;
+ }
+ fds[i].fd = pcap_fileno(pcapConf[i].handle);
}
- fds[i].fd = pcap_fileno(pcapConf[i].handle);
+ tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex);
+ threadkey = g_strdup(req->threadkey);
+ worker = virThreadPoolNewFull(1, 1, 0, virNWFilterDHCPDecodeWorker,
+ "dhcp-decode", NULL, req);
}
- tmp = virNetDevGetIndex(req->binding->portdevname, &ifindex);
- threadkey = g_strdup(req->threadkey);
- worker = virThreadPoolNewFull(1, 1, 0,
- virNWFilterDHCPDecodeWorker,
- "dhcp-decode",
- NULL,
- req);
- }
- /* let creator know how well we initialized */
- if (error || !threadkey || tmp < 0 || !worker ||
- ifindex != req->ifindex) {
- virErrorPreserveLast(&req->threadError);
- req->threadStatus = THREAD_STATUS_FAIL;
- } else {
- req->threadStatus = THREAD_STATUS_OK;
- }
-
- virCondSignal(&req->threadStatusCond);
+ /* let creator know how well we initialized */
+ if (error || !threadkey || tmp < 0 || !worker || ifindex != req->ifindex) {
+ virErrorPreserveLast(&req->threadError);
+ req->threadStatus = THREAD_STATUS_FAIL;
+ } else {
+ req->threadStatus = THREAD_STATUS_OK;
+ }
- virNWFilterSnoopReqUnlock(req);
+ virCondSignal(&req->threadStatusCond);
+ }
if (req->threadStatus != THREAD_STATUS_OK)
goto cleanup;
@@ -1362,12 +1297,10 @@ virNWFilterDHCPSnoopThread(void *req0)
tmp = -1;
/* protect req->binding->portdevname */
- virNWFilterSnoopReqLock(req);
-
- if (req->binding->portdevname)
- tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex);
-
- virNWFilterSnoopReqUnlock(req);
+ VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
+ if (req->binding->portdevname)
+ tmp = virNetDevValidateConfig(req->binding->portdevname, NULL, ifindex);
+ }
if (tmp <= 0) {
error = true;
@@ -1378,20 +1311,17 @@ virNWFilterDHCPSnoopThread(void *req0)
g_clear_pointer(&pcapConf[i].handle, pcap_close);
/* protect req->binding->portdevname */
- virNWFilterSnoopReqLock(req);
-
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("interface '%s' failing; "
- "reopening"),
- req->binding->portdevname);
- if (req->binding->portdevname)
- pcapConf[i].handle =
- virNWFilterSnoopDHCPOpen(req->binding->portdevname,
- &req->binding->mac,
- pcapConf[i].filter,
- pcapConf[i].dir);
-
- virNWFilterSnoopReqUnlock(req);
+ VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("interface '%s' failing; reopening"),
+ req->binding->portdevname);
+ if (req->binding->portdevname)
+ pcapConf[i].handle =
+ virNWFilterSnoopDHCPOpen(req->binding->portdevname,
+ &req->binding->mac,
+ pcapConf[i].filter,
+ pcapConf[i].dir);
+ }
if (!pcapConf[i].handle) {
error = true;
@@ -1448,16 +1378,14 @@ virNWFilterDHCPSnoopThread(void *req0)
/* protect IfNameToKey */
VIR_WITH_MUTEX_LOCK_GUARD(&virNWFilterSnoopState.snoopLock) {
/* protect req->binding->portdevname & req->threadkey */
- virNWFilterSnoopReqLock(req);
-
- virNWFilterSnoopCancel(&req->threadkey);
-
- ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
- req->binding->portdevname));
+ VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
+ virNWFilterSnoopCancel(&req->threadkey);
- g_clear_pointer(&req->binding->portdevname, g_free);
+ ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
+ req->binding->portdevname));
- virNWFilterSnoopReqUnlock(req);
+ g_clear_pointer(&req->binding->portdevname, g_free);
+ }
}
cleanup:
@@ -1497,6 +1425,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
virNWFilterVarValue *dhcpsrvrs;
bool threadPuts = false;
VIR_LOCK_GUARD lock = { NULL };
+ VIR_LOCK_GUARD lockReq = { NULL };
virNWFilterSnoopIFKeyFMT(ifkey, binding->owneruuid, &binding->mac);
@@ -1564,7 +1493,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
}
/* prevent thread from holding req */
- virNWFilterSnoopReqLock(req);
+ lockReq = virLockGuardLock(&req->lock);
if (virThreadCreateFull(&thread, false, virNWFilterDHCPSnoopThread,
"dhcp-snoop", false, req) != 0) {
@@ -1605,8 +1534,6 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
goto exit_snoop_cancel;
}
- virNWFilterSnoopReqUnlock(req);
-
/* do not 'put' the req -- the thread will do this */
return 0;
@@ -1614,7 +1541,7 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriver *techdriver,
exit_snoop_cancel:
virNWFilterSnoopCancel(&req->threadkey);
exit_snoopreq_unlock:
- virNWFilterSnoopReqUnlock(req);
+ virLockGuardUnlock(&lockReq);
exit_rem_ifnametokey:
virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey, binding->portdevname);
exit_snoopunlock:
@@ -1705,12 +1632,11 @@ virNWFilterSnoopPruneIter(const void *payload,
const void *data G_GNUC_UNUSED)
{
virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload;
- bool del_req;
/* clean up orphaned, expired leases */
/* protect req->threadkey */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
if (!req->threadkey)
virNWFilterSnoopReqLeaseTimerRun(req);
@@ -1718,11 +1644,7 @@ virNWFilterSnoopPruneIter(const void *payload,
/*
* have the entry removed if it has no leases and no one holds a ref
*/
- del_req = ((req->start == NULL) && (g_atomic_int_get(&req->refctr) == 0));
-
- virNWFilterSnoopReqUnlock(req);
-
- return del_req;
+ return ((req->start == NULL) && (g_atomic_int_get(&req->refctr) == 0));
}
/*
@@ -1739,12 +1661,11 @@ virNWFilterSnoopSaveIter(void *payload,
virNWFilterSnoopIPLease *ipl;
/* protect req->start */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
for (ipl = req->start; ipl; ipl = ipl->next)
ignore_value(virNWFilterSnoopLeaseFileWrite(tfd, req->ifkey, ipl));
- virNWFilterSnoopReqUnlock(req);
return 0;
}
@@ -1900,7 +1821,7 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
virNWFilterSnoopReq *req = (virNWFilterSnoopReq *)payload;
/* protect req->binding->portdevname */
- virNWFilterSnoopReqLock(req);
+ VIR_LOCK_GUARD lock = virLockGuardLock(&req->lock);
if (req->binding && req->binding->portdevname) {
ignore_value(virHashRemoveEntry(virNWFilterSnoopState.ifnameToKey,
@@ -1916,8 +1837,6 @@ virNWFilterSnoopRemAllReqIter(const void *payload,
g_clear_pointer(&req->binding->portdevname, g_free);
}
- virNWFilterSnoopReqUnlock(req);
-
/* removal will call virNWFilterSnoopCancel() */
return 1;
}
@@ -1999,14 +1918,12 @@ virNWFilterDHCPSnoopEnd(const char *ifname)
}
/* protect req->binding->portdevname & req->threadkey */
- virNWFilterSnoopReqLock(req);
-
- /* keep valid lease req; drop interface association */
- virNWFilterSnoopCancel(&req->threadkey);
+ VIR_WITH_MUTEX_LOCK_GUARD(&req->lock) {
+ /* keep valid lease req; drop interface association */
+ virNWFilterSnoopCancel(&req->threadkey);
- g_clear_pointer(&req->binding->portdevname, g_free);
-
- virNWFilterSnoopReqUnlock(req);
+ g_clear_pointer(&req->binding->portdevname, g_free);
+ }
virNWFilterSnoopReqPut(req);
} else { /* free all of them */
--
2.31.1
More information about the libvir-list
mailing list