[libvirt] [PATCH v3 13/28] lock_driver: Introduce VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA

[John Ferlan]

On 08/27/2018 04:08 AM, Michal Privoznik wrote:
> This is a new type of object that lock drivers can handle.
> Currently, it is supported by lockd driver only.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/locking/lock_driver.h         |  2 ++
>  src/locking/lock_driver_lockd.c   | 43 +++++++++++++++++++++++++++++++--------
>  src/locking/lock_driver_sanlock.c |  3 ++-
>  3 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h
> index a9d2041c30..9be0abcfba 100644
> --- a/src/locking/lock_driver.h
> +++ b/src/locking/lock_driver.h
> @@ -51,6 +51,8 @@ typedef enum {
>      VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0,
>      /* A lease against an arbitrary resource */
>      VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE = 1,
> +    /* The resource to be locked is a metadata */
> +    VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA = 2,
>  } virLockManagerResourceType;
>  
>  typedef enum {
> diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c
> index 98953500b7..d7cb183d7a 100644
> --- a/src/locking/lock_driver_lockd.c
> +++ b/src/locking/lock_driver_lockd.c
> @@ -557,6 +557,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>      virLockManagerLockDaemonPrivatePtr priv = lock->privateData;
>      char *newName = NULL;
>      char *newLockspace = NULL;
> +    int newFlags = 0;
>      bool autoCreate = false;
>      int ret = -1;
>  
> @@ -569,7 +570,7 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>      switch (priv->type) {
>      case VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN:
>  
> -        switch (type) {
> +        switch ((virLockManagerResourceType) type) {
>          case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>              if (params || nparams) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -670,6 +671,8 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>                  goto cleanup;
>  
>          }   break;
> +
> +        case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:

I'm still conflicted with Unknown and Unsupported.

>          default:
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Unknown lock manager object type %d for domain lock object"),
> @@ -679,6 +682,29 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>          break;
>  
>      case VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON:
> +        switch ((virLockManagerResourceType) type) {
> +        case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:
> +            if (params || nparams) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                               _("Unexpected parameters for metadata resource"));
> +                goto cleanup;
> +            }
> +            if (VIR_STRDUP(newLockspace, "") < 0 ||
> +                VIR_STRDUP(newName, name) < 0)
> +                goto cleanup;
> +            newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_METADATA;
> +            break;
> +
> +        case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
> +        case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE:

Again Unknown and Unsupported...

> +        default:
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Unknown lock manager object type %d for daemon lock object"),
> +                           type);
> +            goto cleanup;
> +        }
> +        break;
> +
>      default:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Unknown lock manager object type %d"),
> @@ -686,19 +712,18 @@ static int virLockManagerLockDaemonAddResource(virLockManagerPtr lock,
>          goto cleanup;
>      }
>  
> +    if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)
> +        newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED;
> +
> +    if (autoCreate)

Interstingly enough, @newFlags is adjusted in the new case and we could
do the same in the existing case instead of setting @autoCreate, just
set the newFlags. Of course I'm quite aware that this could have been
done in a separate patch too. IOW: I could easily support removing
@autoCreate...

> +        newFlags |= VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
> +
>      if (VIR_EXPAND_N(priv->resources, priv->nresources, 1) < 0)
>          goto cleanup;
>  
>      VIR_STEAL_PTR(priv->resources[priv->nresources-1].lockspace, newLockspace);
>      VIR_STEAL_PTR(priv->resources[priv->nresources-1].name, newName);
> -
> -    if (flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)
> -        priv->resources[priv->nresources-1].flags |=
> -            VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_SHARED;
> -
> -    if (autoCreate)
> -        priv->resources[priv->nresources-1].flags |=
> -            VIR_LOCK_SPACE_PROTOCOL_ACQUIRE_RESOURCE_AUTOCREATE;
> +    priv->resources[priv->nresources-1].flags = newFlags;
>  
>      ret = 0;
>   cleanup:
> diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
> index fe422d3be6..9393e7d9a2 100644
> --- a/src/locking/lock_driver_sanlock.c
> +++ b/src/locking/lock_driver_sanlock.c
> @@ -815,7 +815,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
>      if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY)
>          return 0;
>  
> -    switch (type) {
> +    switch ((virLockManagerResourceType) type) {
>      case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK:
>          if (driver->autoDiskLease) {
>              if (virLockManagerSanlockAddDisk(driver, lock, name, nparams, params,
> @@ -839,6 +839,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock,
>              return -1;
>          break;
>  
> +    case VIR_LOCK_MANAGER_RESOURCE_TYPE_METADATA:

Conflict continues.

>      default:
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Unknown lock manager object type %d for domain lock object"),
> 

So after all that - I guess I can accept that Unknown will be used and a
separate Unsupported isn't necessary.

However, I do think removing @autoCreate is worthwhile especially since
it's specific to TYPE_DOMAIN and TYPE_DISK. So with that, for the logic
at least regardless if the exact location changes as a result of motion
that may happen prior to this...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John