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@xxxxxxxxxx>
> ---
> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
