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

[John Ferlan]

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 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:
> 
> 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
>