8:38 a.m.
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(a)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