On Wed, Mar 09, 2022 at 12:02:21PM +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/nwfilter/nwfilter_driver.c | 83 +++++++++++++--------------------- 1 file changed, 32 insertions(+), 51 deletions(-)
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index 8eea9e5805..12bbbc661f 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -57,15 +57,6 @@ static int nwfilterStateReload(void);
static virMutex mutex = VIR_MUTEX_INITIALIZER;
-static void nwfilterDriverLock(void) -{ - virMutexLock(&mutex); -} -static void nwfilterDriverUnlock(void) -{ - virMutexUnlock(&mutex); -} - #ifdef WITH_FIREWALLD
static void @@ -204,6 +195,7 @@ nwfilterStateInitialize(bool privileged, virStateInhibitCallback callback G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED) { + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); GDBusConnection *sysbus = NULL;
if (root != NULL) { @@ -230,8 +222,6 @@ nwfilterStateInitialize(bool privileged, if (!privileged) return VIR_DRV_STATE_INIT_SKIPPED;
- nwfilterDriverLock(); - driver->stateDir = g_strdup(RUNSTATEDIR "/libvirt/nwfilter");
if (g_mkdir_with_parents(driver->stateDir, S_IRWXU) < 0) { @@ -290,13 +280,10 @@ nwfilterStateInitialize(bool privileged, if (virNWFilterBuildAll(driver, false) < 0) goto error;
- nwfilterDriverUnlock(); - return VIR_DRV_STATE_INIT_COMPLETE;
error: - nwfilterDriverUnlock(); - nwfilterStateCleanup(); + nwfilterStateCleanupLocked();
return VIR_DRV_STATE_INIT_ERROR;
@@ -335,16 +322,15 @@ nwfilterStateReload(void) /* shut down all threads -- they will be restarted if necessary */ virNWFilterLearnThreadsTerminate(true);
- nwfilterDriverLock(); - virNWFilterWriteLockFilterUpdates(); - - virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + virNWFilterWriteLockFilterUpdates();
- virNWFilterUnlockFilterUpdates(); + virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
- virNWFilterBuildAll(driver, false); + virNWFilterUnlockFilterUpdates();
- nwfilterDriverUnlock(); + virNWFilterBuildAll(driver, false); + }
return 0; } @@ -422,13 +408,13 @@ static virNWFilterPtr nwfilterLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; virNWFilterPtr nwfilter = NULL;
- nwfilterDriverLock(); - obj = nwfilterObjFromNWFilter(uuid); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = nwfilterObjFromNWFilter(uuid); + }
if (!obj) return NULL; @@ -449,13 +435,13 @@ static virNWFilterPtr nwfilterLookupByName(virConnectPtr conn, const char *name) { - virNWFilterObj *obj; + virNWFilterObj *obj = NULL; virNWFilterDef *def; virNWFilterPtr nwfilter = NULL;
- nwfilterDriverLock(); - obj = virNWFilterObjListFindByName(driver->nwfilters, name); - nwfilterDriverUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + obj = virNWFilterObjListFindByName(driver->nwfilters, name); + }
if (!obj) { virReportError(VIR_ERR_NO_NWFILTER, @@ -491,17 +477,16 @@ nwfilterConnectListNWFilters(virConnectPtr conn, char **const names, int maxnames) { - int nnames; + VIR_LOCK_GUARD lock = { NULL };
This is introducing a 3rd locking pattern, not used in any other conversions until now, which feels undesirable to me...
if (virConnectListNWFiltersEnsureACL(conn) < 0) return -1;
- nwfilterDriverLock(); - nnames = virNWFilterObjListGetNames(driver->nwfilters, conn, - virConnectListNWFiltersCheckACL, - names, maxnames); - nwfilterDriverUnlock(); - return nnames; + lock = virLockGuardLock(&mutex); + + return virNWFilterObjListGetNames(driver->nwfilters, conn, + virConnectListNWFiltersCheckACL, + names, maxnames);
...why isn't this just using VIR_WITH_MUTEX_LOCK_GUARD if we don't want the critical section to cover the entire method. Same point for any other case of the same pattern through this series. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|