[libvirt] [PATCH 1/7] util: Alter virObjectLockRead to return status

[Daniel P. Berrange]

On Fri, Jul 28, 2017 at 12:38:55PM -0400, John Ferlan wrote:
> Rather than ignore errors, let's have virObjectLockRead check for
> the correct usage and issue an error when not properly used so
> so that we don't run into situations where the resource we think
> we're locking really isn't locked because the void input parameter
> wasn't valid.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/virdomainobjlist.c | 27 ++++++++++++++++++---------
>  src/util/virobject.c        |  6 +++++-
>  src/util/virobject.h        |  2 +-
>  3 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 1d027a4..fed4029 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -118,7 +118,8 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms,
>                                   bool ref)
>  {
>      virDomainObjPtr obj;
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return NULL;
>      obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL);
>      if (ref) {
>          virObjectRef(obj);
> @@ -160,7 +161,8 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      virDomainObjPtr obj;
>  
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return NULL;
>      virUUIDFormat(uuid, uuidstr);
>  
>      obj = virHashLookup(doms->objs, uuidstr);
> @@ -204,7 +206,8 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
>  {
>      virDomainObjPtr obj;
>  
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return NULL;
>      obj = virHashLookup(doms->objsName, name);
>      virObjectRef(obj);
>      virObjectUnlock(doms);
> @@ -653,7 +656,8 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms,
>                               virConnectPtr conn)
>  {
>      struct virDomainObjListData data = { filter, conn, active, 0 };
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return -1;
>      virHashForEach(doms->objs, virDomainObjListCount, &data);
>      virObjectUnlock(doms);
>      return data.count;
> @@ -697,7 +701,8 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms,
>  {
>      struct virDomainIDData data = { filter, conn,
>                                      0, maxids, ids };
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return -1;
>      virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data);
>      virObjectUnlock(doms);
>      return data.numids;
> @@ -751,7 +756,8 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms,
>      struct virDomainNameData data = { filter, conn,
>                                        0, 0, maxnames, names };
>      size_t i;
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return -1;
>      virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data);
>      virObjectUnlock(doms);
>      if (data.oom) {
> @@ -792,7 +798,8 @@ virDomainObjListForEach(virDomainObjListPtr doms,
>      struct virDomainListIterData data = {
>          callback, opaque, 0,
>      };
> -    virObjectLockRead(doms);
> +    if (virObjectLockRead(doms) < 0)
> +        return -1;
>      virHashForEach(doms->objs, virDomainObjListHelper, &data);
>      virObjectUnlock(doms);
>      return data.ret;
> @@ -925,7 +932,8 @@ virDomainObjListCollect(virDomainObjListPtr domlist,
>  {
>      struct virDomainListData data = { NULL, 0 };
>  
> -    virObjectLockRead(domlist);
> +    if (virObjectLockRead(domlist) < 0)
> +        return -1;
>      sa_assert(domlist->objs);
>      if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) {
>          virObjectUnlock(domlist);
> @@ -962,7 +970,8 @@ virDomainObjListConvert(virDomainObjListPtr domlist,
>      *nvms = 0;
>      *vms = NULL;
>  
> -    virObjectLockRead(domlist);
> +    if (virObjectLockRead(domlist) < 0)
> +        return -1;
>      for (i = 0; i < ndoms; i++) {
>          virDomainPtr dom = doms[i];
>  
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index b1bb378..73de4d3 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -410,17 +410,21 @@ virObjectLock(void *anyobj)
>   * The object must be unlocked before releasing this
>   * reference.
>   */
> -void
> +int

I'm NACK on this return value change.

>  virObjectLockRead(void *anyobj)
>  {
>      if (virObjectIsClass(anyobj, virObjectRWLockableClass)) {
>          virObjectRWLockablePtr obj = anyobj;
>          virRWLockRead(&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 should just be simplified to

  virObjectLockRead(void *anyobj)
  {
     virObjectRWLockablePtr obj = anyobj;
     virRWLockRead(&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 :|