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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
