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

[...] > /** > * 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 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.

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.