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

[John Ferlan]

On 07/25/2017 03:45 AM, Michal Privoznik wrote:
> 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().
> 

Consider passing a non virObject (such as what happened during an early
change to the code for me - a virHashTablePtr) to virObjectLock...

In any case, we'll have to be more vigilant now to know that only
ObjList @obj's for virdomainobjlist can use this new API. Longer term
I'll adjust to use them.

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

There are some ugly ways to handle this including using some kind of
macro for virObjectLock that would force an abort of some sort. We have
been "fortunate" to have well behaved and reviewed code, but with two
lock API's now it's just one of those things to keep track of for reviews.

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

1. Less code and a lock that could check status... I understand not
wanting to modify 10 callers to check status, but modifying two and thus
making it clearer to the caller that these are "different" might not be
a bad thing.

2. The "default" action being more consumers probably will use the Read
locks (as I believe is the premise/impetus of the series). The
Add/Remove would seemingly be used less and thus the exception. So why
not have the default for ObjList/virObjectRWLockableClass be to have the
virObjectLock use a virRWLockRead instead of virRWLockWrite?


What's done is done - I obviously have ulterior motives to not wanting
virobject to change that much, but I've been considering the downside of
the code that has void functions that really don't return a lock on the
object if the wrong object was passed.

John