On 09/03/2018 11:13 AM, Michal Privoznik wrote:
On 08/30/2018 11:34 PM, John Ferlan wrote:
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@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:
As I explain in one of my previous replies, users are not really expected to see this message. Is merely for us to avoid broken code pattern. Even if it so happens that broken code slips through review, what difference does it make for users to see "Unsupported lock manager object type" vs "Unknown lock manager object type"? They'll file a bug and we will notice immediately what is the problem when looking into the code (we will notice it because the error message logs type number).
Consider my your early warning bz then ;-). It doesn't really matter that much... Maybe it's more of an "Unsupported" message regardless of whether it's META or 'default'. At first I considered noting the Enum range error, but it's not the same here. I'll leave it as a design decision and move on. John
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...
Okay.
Michal