[libvirt] [PATCH 2/7] util: Introduce and use virObjectLockWrite

[Daniel P. Berrange]

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 <jferlan at redhat.com>
> ---
>  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 :|