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

[John Ferlan]

[...]

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

John

>  
>  /**
>   * virObjectUnlock:
> - * @anyobj: any instance of virObjectLockablePtr
> + * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>   *
> - * Release a lock on @anyobj. The lock must have been
> - * acquired by virObjectLock.
> + * Release a lock on @anyobj. The lock must have been acquired by
> + * virObjectLock or virObjectLockRead.
>   */
>  void
>  virObjectUnlock(void *anyobj)
>  {
> -    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
> -
> -    if (!obj)
> -        return;
> -
> -    virMutexUnlock(&obj->lock);
> +    if (virObjectIsClass(anyobj, virObjectLockableClass)) {
> +        virObjectLockablePtr obj = anyobj;
> +        virMutexUnlock(&obj->lock);
> +    } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> +        virObjectRWLockablePtr obj = anyobj;
> +        virRWLockUnlock(&obj->lock);
> +    } else {
> +        virObjectPtr obj = anyobj;
> +        VIR_WARN("Object %p (%s) is not a virObjectLockable "
> +                 "nor virObjectRWLockable instance",
> +                 anyobj, obj ? obj->klass->name : "(unknown)");
> +    }
>  }
>  
[...]