[libvirt] [PATCH 2/3] virobject: Introduce virObjectRWLockable

[Michal Privoznik]

On 07/24/2017 05:22 PM, John Ferlan wrote:
> [...]
> 
>>  /**
>>   * virObjectLock:
>> - * @anyobj: any instance of virObjectLockablePtr
>> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>>   *
>> - * Acquire a lock on @anyobj. The lock must be
>> - * released by virObjectUnlock.
>> + * Acquire a lock on @anyobj. The lock must be released by
>> + * virObjectUnlock. In case the passed object is instance of
>> + * virObjectRWLockable a write lock is acquired.
>>   *
>>   * The caller is expected to have acquired a reference
>>   * on the object before locking it (eg virObjectRef).
>> @@ -340,31 +382,69 @@ virObjectGetLockableObj(void *anyobj)
>>  void
>>  virObjectLock(void *anyobj)
>>  {
>> -    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
>> +    if (virObjectIsClass(anyobj, virObjectLockableClass)) {
>> +        virObjectLockablePtr obj = anyobj;
>> +        virMutexLock(&obj->lock);
>> +    } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>> +        virObjectRWLockablePtr obj = anyobj;
>> +        virRWLockWrite(&obj->lock);
>> +    } else {
>> +        virObjectPtr obj = anyobj;
>> +        VIR_WARN("Object %p (%s) is not a virObjectLockable "
>> +                 "nor virObjectRWLockable instance",
>> +                 anyobj, obj ? obj->klass->name : "(unknown)");
>> +    }
>> +}
>>  
>> -    if (!obj)
>> -        return;
>>  
>> -    virMutexLock(&obj->lock);
>> +/**
>> + * virObjectLockRead:
>> + * @anyobj: any instance of virObjectRWLockablePtr
>> + *
>> + * Acquire a read lock on @anyobj. The lock must be
>> + * released by virObjectUnlock.
>> + *
>> + * The caller is expected to have acquired a reference
>> + * on the object before locking it (eg virObjectRef).
>> + * The object must be unlocked before releasing this
>> + * reference.
>> + */
>> +void
>> +virObjectLockRead(void *anyobj)
>> +{
>> +    if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>> +        virObjectRWLockablePtr obj = anyobj;
>> +        virRWLockRead(&obj->lock);
>> +    } else {
>> +        virObjectPtr obj = anyobj;
>> +        VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
>> +                 anyobj, obj ? obj->klass->name : "(unknown)");
>> +    }
>>  }
>>  
> 
> Hopefully no one "confuses" which object is which or no one starts using
> virObjectLockRead for a virObjectLockable and expects that read lock to
> be in place (or error out) or gasp actually wait for a write lock to
> release the lock as this does not error out.

This could have already happened if one would pass bare virObject to
virObjectLock().

> 
> This is a danger in the long term assumption of having a void function
> that has callers that can make the assumption that upon return the Lock
> really was taken.

Well, I agree that this is one more thing to be cautious about, but no
more than making sure object passed to virObjectLock() is virObjectLockable.

> 
> Perhaps the better way to attack this would have been to create a
> virObjectLockWrite() function called only from vir*ObjListAdd and
> vir*ObjListRemove leaving the other virObjectLock()'s in tact and having
> the virObjectLock() code make the decision over whether to use the
> virRWLockRead or virMutexLock depending on the object class.

What's the difference? If virObjectLock() does what you're suggesting
(what indeed is implemented too), what's the point in having
virObjectLockWrite()?

Michal