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

[Michal Privoznik]

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).

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