Now that virObjectLockWrite exists to handle the virObjectRWLockable
objects, let's restore virObjectLock to only handle virObjectLockable
type locks. There still exists the possibility that the input @anyobj
isn't a valid object and the resource isn't truly locked, but that
also exists before commit id '77f4593b'.

This also restores some logic that commit id '77f4593b' removed
with respect to a common code path that commit id '10c2bb2b' had
introduced as virObjectGetLockableObj.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/util/virobject.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/src/util/virobject.c b/src/util/virobject.c
index c48f88c..f9047a1 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -367,13 +367,28 @@ virObjectRef(void *anyobj)
 }
 
 
+static virObjectLockablePtr
+virObjectGetLockableObj(void *anyobj)
+{
+    virObjectPtr obj;
+
+    if (virObjectIsClass(anyobj, virObjectLockableClass))
+        return anyobj;
+
+    obj = anyobj;
+    VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
+              anyobj, obj ? obj->klass->name : "(unknown)");
+
+    return NULL;
+}
+
+
 /**
  * virObjectLock:
  * @anyobj: any instance of virObjectLockable or virObjectRWLockable
  *
  * 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.
+ * virObjectUnlock.
  *
  * The caller is expected to have acquired a reference
  * on the object before locking it (eg virObjectRef).
@@ -383,18 +398,12 @@ virObjectRef(void *anyobj)
 void
 virObjectLock(void *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)");
-    }
+    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+    if (!obj)
+        return;
+
+    virMutexLock(&obj->lock);
 }
 
 
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
> Now that virObjectLockWrite exists to handle the virObjectRWLockable
> objects, let's restore virObjectLock to only handle virObjectLockable
> type locks. There still exists the possibility that the input @anyobj
> isn't a valid object and the resource isn't truly locked, but that
> also exists before commit id '77f4593b'.
> 
> This also restores some logic that commit id '77f4593b' removed
> with respect to a common code path that commit id '10c2bb2b' had
> introduced as virObjectGetLockableObj.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/util/virobject.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
> Now that virObjectLockWrite exists to handle the virObjectRWLockable
> objects, let's restore virObjectLock to only handle virObjectLockable
> type locks. There still exists the possibility that the input @anyobj
> isn't a valid object and the resource isn't truly locked, but that
> also exists before commit id '77f4593b'.
> 
> This also restores some logic that commit id '77f4593b' removed
> with respect to a common code path that commit id '10c2bb2b' had
> introduced as virObjectGetLockableObj.
> 
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---
>  src/util/virobject.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index c48f88c..f9047a1 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -367,13 +367,28 @@ virObjectRef(void *anyobj)
>  }
>  
>  
> +static virObjectLockablePtr
> +virObjectGetLockableObj(void *anyobj)
> +{
> +    virObjectPtr obj;
> +
> +    if (virObjectIsClass(anyobj, virObjectLockableClass))
> +        return anyobj;
> +
> +    obj = anyobj;
> +    VIR_WARN("Object %p (%s) is not a virObjectLockable instance",
> +              anyobj, obj ? obj->klass->name : "(unknown)");
> +
> +    return NULL;
> +}
> +
> +
>  /**
>   * virObjectLock:
>   * @anyobj: any instance of virObjectLockable or virObjectRWLockable
>   *
>   * 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.
> + * virObjectUnlock.
>   *
>   * The caller is expected to have acquired a reference
>   * on the object before locking it (eg virObjectRef).
> @@ -383,18 +398,12 @@ virObjectRef(void *anyobj)
>  void
>  virObjectLock(void *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)");
> -    }
> +    virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
> +
> +    if (!obj)
> +        return;
> +
> +    virMutexLock(&obj->lock);

Again I'd prefer to see

        virObjectLockablePtr obj = anyobj;
        virMutexLock(&obj->lock);

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list