Rather than overload virObjectUnlock as commit id '77f4593b' has
done, create a separate virObjectRWUnlock API that will force the
consumers to make the proper decision regarding unlocking the
RWLock's. This restores the virObjectUnlock code to using the
virObjectGetLockableObj as well.
Signed-off-by: John Ferlan <jferlan@redhat.com>
---
src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------
src/libvirt_private.syms | 1 +
src/util/virobject.c | 41 +++++++++++++++++++++++++++--------------
src/util/virobject.h | 4 ++++
4 files changed, 50 insertions(+), 32 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 08a51cc..85f5434 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -123,7 +123,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
if (ref) {
virObjectRef(obj);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
}
if (obj) {
virObjectLock(obj);
@@ -135,7 +135,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
}
}
if (!ref)
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return obj;
}
@@ -168,7 +168,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
obj = virHashLookup(doms->objs, uuidstr);
if (ref) {
virObjectRef(obj);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
}
if (obj) {
virObjectLock(obj);
@@ -180,7 +180,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
}
}
if (!ref)
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return obj;
}
@@ -210,7 +210,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
return NULL;
obj = virHashLookup(doms->objsName, name);
virObjectRef(obj);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -333,7 +333,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
if (virObjectLockWrite(doms) < 0)
return NULL;
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return ret;
}
@@ -361,7 +361,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virHashRemoveEntry(doms->objsName, dom->def->name);
virObjectUnlock(dom);
virObjectUnref(dom);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
}
@@ -430,7 +430,7 @@ virDomainObjListRename(virDomainObjListPtr doms,
ret = 0;
cleanup:
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
VIR_FREE(old_name);
return ret;
}
@@ -622,7 +622,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
}
VIR_DIR_CLOSE(dir);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return ret;
}
@@ -669,7 +669,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListCount, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return data.count;
}
@@ -714,7 +714,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return data.numids;
}
@@ -769,7 +769,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
if (data.oom) {
for (i = 0; i < data.numnames; i++)
VIR_FREE(data.names[i]);
@@ -811,7 +811,7 @@ virDomainObjListForEach(virDomainObjListPtr doms,
if (virObjectLockRead(doms) < 0)
return -1;
virHashForEach(doms->objs, virDomainObjListHelper, &data);
- virObjectUnlock(doms);
+ virObjectRWUnlock(doms);
return data.ret;
}
@@ -946,12 +946,12 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
return -1;
sa_assert(domlist->objs);
if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
return -1;
}
virHashForEach(domlist->objs, virDomainObjListCollectIterator, &data);
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
virDomainObjListFilter(&data.vms, &data.nvms, conn, filter, flags);
@@ -991,7 +991,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
if (skip_missing)
continue;
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
virReportError(VIR_ERR_NO_DOMAIN,
_("no domain with matching uuid '%s' (%s)"),
uuidstr, dom->name);
@@ -1001,12 +1001,12 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
virObjectRef(vm);
if (VIR_APPEND_ELEMENT(*vms, *nvms, vm) < 0) {
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
virObjectUnref(vm);
goto error;
}
}
- virObjectUnlock(domlist);
+ virObjectRWUnlock(domlist);
sa_assert(*vms);
virDomainObjListFilter(vms, nvms, conn, filter, flags);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f1a6510..5e6b58c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2310,6 +2310,7 @@ virObjectLockWrite;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
+virObjectRWUnlock;
virObjectUnlock;
virObjectUnref;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 0db98c3..6f786ce 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -480,26 +480,39 @@ virObjectLockWrite(void *anyobj)
/**
* virObjectUnlock:
- * @anyobj: any instance of virObjectLockable or virObjectRWLockable
+ * @anyobj: any instance of virObjectLockable
*
* Release a lock on @anyobj. The lock must have been acquired by
- * virObjectLock or virObjectLockRead.
+ * virObjectLock.
*/
void
virObjectUnlock(void *anyobj)
{
- 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)");
- }
+ virObjectLockablePtr obj = virObjectGetLockableObj(anyobj);
+
+ if (!obj)
+ return;
+
+ virMutexUnlock(&obj->lock);
+}
+
+
+/**
+ * virObjectRWUnlock:
+ * @anyobj: any instance of virObjectRWLockable
+ *
+ * Release a lock on @anyobj. The lock must have been acquired by
+ * virObjectLockRead or virObjectLockWrite.
+ */
+void
+virObjectRWUnlock(void *anyobj)
+{
+ virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj);
+
+ if (!obj)
+ return;
+
+ virRWLockUnlock(&obj->lock);
}
diff --git a/src/util/virobject.h b/src/util/virobject.h
index f0d1f97..bd0fbb8 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -137,6 +137,10 @@ virObjectUnlock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
void
+virObjectRWUnlock(void *lockableobj)
+ ATTRIBUTE_NONNULL(1);
+
+void
virObjectListFree(void *list);
void
--
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:59PM -0400, John Ferlan wrote: > Rather than overload virObjectUnlock as commit id '77f4593b' has > done, create a separate virObjectRWUnlock API that will force the > consumers to make the proper decision regarding unlocking the > RWLock's. This restores the virObjectUnlock code to using the > virObjectGetLockableObj as well. > > Signed-off-by: John Ferlan <jferlan@redhat.com> > --- > src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------ > src/libvirt_private.syms | 1 + > src/util/virobject.c | 41 +++++++++++++++++++++++++++-------------- > src/util/virobject.h | 4 ++++ > 4 files changed, 50 insertions(+), 32 deletions(-) The virObjectLockRead and virObjectLockWrite should be probably renamed to virObjectRWLockRead and virObjectRWLockWrite for consistency with the virObjectRWUnlock. Reviewed-by: Pavel Hrdina <phrdina@redhat.com> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/28/2017 07:07 PM, Pavel Hrdina wrote: > On Fri, Jul 28, 2017 at 12:38:59PM -0400, John Ferlan wrote: >> Rather than overload virObjectUnlock as commit id '77f4593b' has >> done, create a separate virObjectRWUnlock API that will force the >> consumers to make the proper decision regarding unlocking the >> RWLock's. This restores the virObjectUnlock code to using the >> virObjectGetLockableObj as well. >> >> Signed-off-by: John Ferlan <jferlan@redhat.com> >> --- >> src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------ >> src/libvirt_private.syms | 1 + >> src/util/virobject.c | 41 +++++++++++++++++++++++++++-------------- >> src/util/virobject.h | 4 ++++ >> 4 files changed, 50 insertions(+), 32 deletions(-) > > The virObjectLockRead and virObjectLockWrite should be probably renamed > to virObjectRWLockRead and virObjectRWLockWrite for consistency with the > virObjectRWUnlock. > > Reviewed-by: Pavel Hrdina <phrdina@redhat.com> This is where I disagree. While you may dislike lock() polymorphism, unlock() is used widely in literature for both mutexes and RW locks. And it makes sense because they are fundamentally the same. I'm not going to NACK this patch only because I find it impolite. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.