[libvirt] [PATCH 1/3] lock_driver_sanlock: Avoid global driver variable whenever possible

[John Ferlan]

On 09/15/2016 10:35 AM, Michal Privoznik wrote:
> Global variables are bad, we should avoid using them.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/locking/lock_driver_sanlock.c | 42 ++++++++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
> index 579f696..1ff1abd 100644
> --- a/src/locking/lock_driver_sanlock.c
> +++ b/src/locking/lock_driver_sanlock.c
> @@ -80,7 +80,7 @@ struct _virLockManagerSanlockDriver {
>      gid_t group;
>  };
>  
> -static virLockManagerSanlockDriver *driver;
> +static virLockManagerSanlockDriverPtr sanlockDriver;
>  
>  struct _virLockManagerSanlockPrivate {
>      const char *vm_uri;
> @@ -100,7 +100,8 @@ struct _virLockManagerSanlockPrivate {
>  /*
>   * sanlock plugin for the libvirt virLockManager API
>   */
> -static int virLockManagerSanlockLoadConfig(const char *configFile)
> +static int virLockManagerSanlockLoadConfig(virLockManagerSanlockDriverPtr driver,
> +                                           const char *configFile)

static int
virLock*

Ironically you did this for the next one...

>  {
>      virConfPtr conf;
>      int ret = -1;
> @@ -161,7 +162,8 @@ static int virLockManagerSanlockLoadConfig(const char *configFile)
>  /* How many times try adding a lockspace? */
>  #define LOCKSPACE_RETRIES 10
>  
> -static int virLockManagerSanlockSetupLockspace(void)
> +static int
> +virLockManagerSanlockSetupLockspace(virLockManagerSanlockDriverPtr driver)
>  {
>      int fd = -1;
>      struct stat st;
> @@ -371,16 +373,20 @@ static int virLockManagerSanlockInit(unsigned int version,
>                                       const char *configFile,
>                                       unsigned int flags)
>  {
> +    virLockManagerSanlockDriverPtr driver;
> +
>      VIR_DEBUG("version=%u configFile=%s flags=%x",
>                version, NULLSTR(configFile), flags);
>      virCheckFlags(0, -1);
>  
> -    if (driver)
> +    if (sanlockDriver)
>          return 0;
>  
> -    if (VIR_ALLOC(driver) < 0)
> +    if (VIR_ALLOC(sanlockDriver) < 0)
>          return -1;
>  
> +    driver = sanlockDriver;
> +
>      driver->requireLeaseForDisks = true;
>      driver->hostID = 0;
>      driver->autoDiskLease = false;
> @@ -392,7 +398,7 @@ static int virLockManagerSanlockInit(unsigned int version,
>          goto error;
>      }
>  
> -    if (virLockManagerSanlockLoadConfig(configFile) < 0)
> +    if (virLockManagerSanlockLoadConfig(driver, configFile) < 0)
>          goto error;
>  
>      if (driver->autoDiskLease && !driver->hostID) {
> @@ -402,7 +408,7 @@ static int virLockManagerSanlockInit(unsigned int version,
>      }
>  
>      if (driver->autoDiskLease) {
> -        if (virLockManagerSanlockSetupLockspace() < 0)
> +        if (virLockManagerSanlockSetupLockspace(driver) < -1)
>              goto error;
>      }
>  
> @@ -415,11 +421,11 @@ static int virLockManagerSanlockInit(unsigned int version,
>  
>  static int virLockManagerSanlockDeinit(void)
>  {
> -    if (!driver)
> +    if (!sanlockDriver)
>          return 0;
>  
> -    VIR_FREE(driver->autoDiskLeasePath);
> -    VIR_FREE(driver);
> +    VIR_FREE(sanlockDriver->autoDiskLeasePath);
> +    VIR_FREE(sanlockDriver);
>  
>      return 0;
>  }
> @@ -438,7 +444,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock,
>  
>      virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1);
>  
> -    if (!driver) {
> +    if (!sanlockDriver) {

Interesting that this is the one API which checks - every other API
assumes sanlockDriver would be set since they can assume the Init
function was run.

>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Sanlock plugin is not initialized"));
>          return -1;
> @@ -566,7 +572,8 @@ static int virLockManagerSanlockAddLease(virLockManagerPtr lock,
>  
>  
>  
> -static int virLockManagerSanlockAddDisk(virLockManagerPtr lock,
> +static int virLockManagerSanlockAddDisk(virLockManagerSanlockDriverPtr driver,
> +                                        virLockManagerPtr lock,

static int
virLock*

>                                          const char *name,
>                                          size_t nparams,
>                                          virLockManagerParamPtr params ATTRIBUTE_UNUSED,
> @@ -631,7 +638,8 @@ static int virLockManagerSanlockAddDisk(virLockManagerPtr lock,
>  }
>  
>  
> -static int virLockManagerSanlockCreateLease(struct sanlk_resource *res)
> +static int virLockManagerSanlockCreateLease(virLockManagerSanlockDriverPtr driver,
> + 

static int
virLock*
                                           struct sanlk_resource *res)
>  {
>      int fd = -1;
>      struct stat st;
> @@ -719,6 +727,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
>                                              virLockManagerParamPtr params,
>                                              unsigned int flags)
>  {
> +    virLockManagerSanlockDriverPtr driver = sanlockDriver;

Existing - unlike virLockManagerSanlockNew this code doesn't check for
!sanlockDriver (or !driver) and fail...

>      virLockManagerSanlockPrivatePtr priv = lock->privateData;
>  
>      virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY |
> @@ -738,11 +747,12 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
>      switch (type) {
>      case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>          if (driver->autoDiskLease) {
> -            if (virLockManagerSanlockAddDisk(lock, name, nparams, params,
> +            if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params,
>                                               !!(flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)) < 0)
>                  return -1;
>  
> -            if (virLockManagerSanlockCreateLease(priv->res_args[priv->res_count-1]) < 0)
> +            if (virLockManagerSanlockCreateLease(driver,
> +                                                 priv->res_args[priv->res_count-1]) < 0)
>                  return -1;
>          } else {
>              if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED |
> @@ -882,7 +892,7 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock,


You went away from your model...

    virLockManagerSanlockDriverPtr driver = sanlockDriver;

and use driver... it's also of note that there is not a !driver check
here too.


In any case, the issues are cosmetic and your choice on how to handle.

ACK

John

>  
>      if (priv->res_count == 0 &&
>          priv->hasRWDisks &&
> -        driver->requireLeaseForDisks) {
> +        sanlockDriver->requireLeaseForDisks) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("Read/write, exclusive access, disks were present, but no leases specified"));
>          return -1;
>