On Wed, Mar 09, 2022 at 12:02:21PM +0100, Tim Wiederhake wrote:
> Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx>
> ---
> 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 :|
