Re: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote:
> Instead of making virObjectLock be the entry point for two
> different types of locks, let's create a virObjectLockWrite API
> which will be able to return status and force the (new) consumers
> of the RWLock to make sure the lock is really obtained when the
> "right" object type is passed.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virdomainobjlist.c | 18 ++
> src/libvirt_private.syms| 1 +
> src/util/virobject.c| 32
> src/util/virobject.h| 4
> 4 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index fed4029..08a51cc 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -330,7 +330,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr
> doms,
> {
> virDomainObjPtr ret;
>
> -virObjectLock(doms);
> +if (virObjectLockWrite(doms) < 0)
> +return NULL;
> ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
> virObjectUnlock(doms);
> return ret;
> @@ -352,7 +353,9 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
> virObjectRef(dom);
> virObjectUnlock(dom);
>
> -virObjectLock(doms);
> +/* We really shouldn't ignore this,
> + * but that ship sailed a long time ago */
> +ignore_value(virObjectLockWrite(doms));
> virObjectLock(dom);
> virHashRemoveEntry(doms->objs, uuidstr);
> virHashRemoveEntry(doms->objsName, dom->def->name);
> @@ -397,9 +400,13 @@ virDomainObjListRename(virDomainObjListPtr doms,
> * hold a lock on dom but not refcount it. */
> virObjectRef(dom);
> virObjectUnlock(dom);
> -virObjectLock(doms);
> +rc = virObjectLockWrite(doms);
> virObjectLock(dom);
> virObjectUnref(dom);
> +if (rc < 0) {
> +VIR_FREE(old_name);
> +return ret;
> +}
>
> if (virHashLookup(doms->objsName, new_name) != NULL) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -576,7 +583,10 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
> if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
> return rc;
>
> -virObjectLock(doms);
> +if (virObjectLockWrite(doms) < 0) {
> +VIR_DIR_CLOSE(dir);
> +return -1;
> +}
>
> while ((ret = virDirRead(dir, &entry, configDir)) > 0) {
> virDomainObjPtr dom;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 37b815c..f1a6510 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2306,6 +2306,7 @@ virObjectListFreeCount;
> virObjectLock;
> virObjectLockableNew;
> virObjectLockRead;
> +virObjectLockWrite;
> virObjectNew;
> virObjectRef;
> virObjectRWLockableNew;
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 73de4d3..c48f88c 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -429,6 +429,38 @@ virObjectLockRead(void *anyobj)
>
>
> /**
> + * virObjectLockWrite:
> + * @anyobj: any instance of virObjectRWLockable
> + *
> + * Acquire a write 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.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +virObjectLockWrite(void *anyobj)
> +{
> +if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
> +virObjectRWLockablePtr obj = anyobj;
> +virRWLockWrite(&obj->lock);
> +return 0;
> +} else {
> +virObjectPtr obj = anyobj;
> +VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
> + anyobj, obj ? obj->klass->name : "(unknown)");
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unable to obtain rwlock for object=%p"), anyobj);
> +}
> +return -1;
> +}
IMHO this can be simplified
void
virObjectLockWrite(void *anyobj)
{
virObjectRWLockablePtr obj = anyobj;
virRWLockWrite(&obj->lock);
}
because if some caller has passed in a bogus argument (something not an
object, or an object that's already free), then the virObjectIsClass
is just as likley to crash as the simpler code, and IMHO it is preferrable
to get an immediate crash, than to carry on with a warning mesage on
the console and no lock acquired.
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 2/7] util: Introduce and use virObjectLockWrite
On 07/28/2017 12:59 PM, Pavel Hrdina wrote: > On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote: >> Instead of making virObjectLock be the entry point for two >> different types of locks, let's create a virObjectLockWrite API >> which will be able to return status and force the (new) consumers >> of the RWLock to make sure the lock is really obtained when the >> "right" object type is passed. > > If you remove the return value for the same reason as in the first patch > > Reviewed-by: Pavel Hrdina > Fine again, minority opinion it seems. Still rename from virObjectLockWrite to virObjectRWLockWrite is expect, right (or is that Write) John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
On Fri, Jul 28, 2017 at 12:38:56PM -0400, John Ferlan wrote: > Instead of making virObjectLock be the entry point for two > different types of locks, let's create a virObjectLockWrite API > which will be able to return status and force the (new) consumers > of the RWLock to make sure the lock is really obtained when the > "right" object type is passed. If you remove the return value for the same reason as in the first patch Reviewed-by: Pavel Hrdina signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite
Instead of making virObjectLock be the entry point for two
different types of locks, let's create a virObjectLockWrite API
which will be able to return status and force the (new) consumers
of the RWLock to make sure the lock is really obtained when the
"right" object type is passed.
Signed-off-by: John Ferlan
---
src/conf/virdomainobjlist.c | 18 ++
src/libvirt_private.syms| 1 +
src/util/virobject.c| 32
src/util/virobject.h| 4
4 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index fed4029..08a51cc 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -330,7 +330,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr
doms,
{
virDomainObjPtr ret;
-virObjectLock(doms);
+if (virObjectLockWrite(doms) < 0)
+return NULL;
ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef);
virObjectUnlock(doms);
return ret;
@@ -352,7 +353,9 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
virObjectRef(dom);
virObjectUnlock(dom);
-virObjectLock(doms);
+/* We really shouldn't ignore this,
+ * but that ship sailed a long time ago */
+ignore_value(virObjectLockWrite(doms));
virObjectLock(dom);
virHashRemoveEntry(doms->objs, uuidstr);
virHashRemoveEntry(doms->objsName, dom->def->name);
@@ -397,9 +400,13 @@ virDomainObjListRename(virDomainObjListPtr doms,
* hold a lock on dom but not refcount it. */
virObjectRef(dom);
virObjectUnlock(dom);
-virObjectLock(doms);
+rc = virObjectLockWrite(doms);
virObjectLock(dom);
virObjectUnref(dom);
+if (rc < 0) {
+VIR_FREE(old_name);
+return ret;
+}
if (virHashLookup(doms->objsName, new_name) != NULL) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -576,7 +583,10 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms,
if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0)
return rc;
-virObjectLock(doms);
+if (virObjectLockWrite(doms) < 0) {
+VIR_DIR_CLOSE(dir);
+return -1;
+}
while ((ret = virDirRead(dir, &entry, configDir)) > 0) {
virDomainObjPtr dom;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 37b815c..f1a6510 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2306,6 +2306,7 @@ virObjectListFreeCount;
virObjectLock;
virObjectLockableNew;
virObjectLockRead;
+virObjectLockWrite;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 73de4d3..c48f88c 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -429,6 +429,38 @@ virObjectLockRead(void *anyobj)
/**
+ * virObjectLockWrite:
+ * @anyobj: any instance of virObjectRWLockable
+ *
+ * Acquire a write 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.
+ *
+ * Returns 0 on success, -1 on failure
+ */
+int
+virObjectLockWrite(void *anyobj)
+{
+if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
+virObjectRWLockablePtr obj = anyobj;
+virRWLockWrite(&obj->lock);
+return 0;
+} else {
+virObjectPtr obj = anyobj;
+VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance",
+ anyobj, obj ? obj->klass->name : "(unknown)");
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to obtain rwlock for object=%p"), anyobj);
+}
+return -1;
+}
+
+
+/**
* virObjectUnlock:
* @anyobj: any instance of virObjectLockable or virObjectRWLockable
*
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 99910e0..f0d1f97 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -128,6 +128,10 @@ int
virObjectLockRead(void *lockableobj)
ATTRIBUTE_NONNULL(1);
+int
+virObjectLockWrite(void *lockableobj)
+ATTRIBUTE_NONNULL(1);
+
void
virObjectUnlock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
--
2.9.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
