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@xxxxxxxxxx>
> ---
> 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;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list