[v1] virObject adjustments for common object

[libvirt] [PATCH 5/9] util: Introduce virObjectLockableRecursiveNew

Posted by John Ferlan 8 years, 8 months ago

Introduce a recursive option for the lockable object which calls
virMutexInitRecursive instead of virMutexInit.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virobject.c     | 26 ++++++++++++++++++++++++++
 src/util/virobject.h     |  4 ++++
 3 files changed, 31 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 429b095..9693dc4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2268,6 +2268,7 @@ virObjectListFree;
 virObjectListFreeCount;
 virObjectLock;
 virObjectLockableNew;
+virObjectLockableRecursiveNew;
 virObjectNew;
 virObjectRef;
 virObjectUnlock;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index a1934941..b1fabd7 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -250,6 +250,32 @@ virObjectLockableNew(virClassPtr klass)
 }
 
 
+void *
+virObjectLockableRecursiveNew(virClassPtr klass)
+{
+    virObjectLockablePtr obj;
+
+    if (!virClassIsDerivedFrom(klass, virClassForObjectLockable())) {
+        virReportInvalidArg(klass,
+                            _("Class %s must derive from virObjectLockable"),
+                            virClassName(klass));
+        return NULL;
+    }
+
+    if (!(obj = virObjectNew(klass)))
+        return NULL;
+
+    if (virMutexInitRecursive(&obj->lock) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("Unable to initialize recursive mutex"));
+        virObjectUnref(obj);
+        return NULL;
+    }
+
+    return obj;
+}
+
+
 static void
 virObjectLockableDispose(void *anyobj)
 {
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 89f8050..7df9b47 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -109,6 +109,10 @@ void *
 virObjectLockableNew(virClassPtr klass)
     ATTRIBUTE_NONNULL(1);
 
+void *
+virObjectLockableRecursiveNew(virClassPtr klass)
+    ATTRIBUTE_NONNULL(1);
+
 void
 virObjectLock(void *lockableobj)
     ATTRIBUTE_NONNULL(1);
-- 
2.9.4

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

Re: [libvirt] [PATCH 5/9] util: Introduce virObjectLockableRecursiveNew

Posted by Pavel Hrdina 8 years, 8 months ago

On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
> Introduce a recursive option for the lockable object which calls
> virMutexInitRecursive instead of virMutexInit.

We should avoid using recursive lock because they are dangerous.  I
would rather cleanup the nwfilter code than introducing more stuff for
recursive locks.

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

Re: [libvirt] [PATCH 5/9] util: Introduce virObjectLockableRecursiveNew

Posted by Daniel P. Berrange 8 years, 8 months ago

On Tue, May 30, 2017 at 03:03:14PM +0200, Pavel Hrdina wrote:
> On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
> > Introduce a recursive option for the lockable object which calls
> > virMutexInitRecursive instead of virMutexInit.
> 
> We should avoid using recursive lock because they are dangerous.  I
> would rather cleanup the nwfilter code than introducing more stuff for
> recursive locks.

Last time I looked I came to the conclusion that it wasn't possible to
remove the recursive locking from nwfilter.

I agree about /not/ introducing new usage of recursive locking though.

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

Re: [libvirt] [PATCH 5/9] util: Introduce virObjectLockableRecursiveNew

Posted by John Ferlan 8 years, 8 months ago

On 05/30/2017 09:19 AM, Daniel P. Berrange wrote:
> On Tue, May 30, 2017 at 03:03:14PM +0200, Pavel Hrdina wrote:
>> On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
>>> Introduce a recursive option for the lockable object which calls
>>> virMutexInitRecursive instead of virMutexInit.
>>
>> We should avoid using recursive lock because they are dangerous.  I
>> would rather cleanup the nwfilter code than introducing more stuff for
>> recursive locks.
> 
> Last time I looked I came to the conclusion that it wasn't possible to
> remove the recursive locking from nwfilter.
> 
> I agree about /not/ introducing new usage of recursive locking though.
> 

So this is a chicken/egg problem.... Cannot create a new API to use
recursive locks and  cannot remove the recursive lock without having
adjustments to nwfilter code that would seemingly require self locking
list objects to get hash table elements.

If the comments in src/nwfilter/nwfilter_gentech_driver.c (search on
XXX) could prove to be true, then it would be possible to not have a
recursive lock. Perhaps that loop could be rewritten in virnwfilterobj
to grab a reference to a hash table object while releasing the object
lock that's causing the deadlock.  I haven't yet circled back to
determine whether this is possible yet, but since code in my branches is
at least this far, I can look again.

John

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