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