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