[libvirt PATCH 03/12] nwfilter_driver: Use automatic mutex management

[Daniel P. Berrangé]

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