[libvirt] [PATCH 0/7] Some virObjectRW* adjustments
As indicated in a couple of recent reviews, the new virObjectRWLockable object and API's I believe needed a few adjustments in order to make them better. So I present those thoughts in patch format to be hopefully dicussed and dissected. Essentially this series, will modify the newly added virObjectLockRead to return a status and to have that status managed. Since there were only 9 new callers, this was straightforward. None of the callers actually needed any sort of "divert logic" in order to jump around code that shouldn't do the Unlock now. Although that could be a problem if ever changing other more deeply nested type locks. That's someone else's headache though. Next, introduce and use virObjectLockWrite to be only for the new style RWLocks. This too will return status. Only the vir*Remove caller has a code path that I'm not happy with, but since these type locks have been processing along anyway without technically ensuring they have the lock before doing so, well no harm, no foul. Next, introduce and use virObjectRWUnlock. Rather than overloading the virObjectUnlock, let's just keep each caller doing it's one thing to be sure that the calling code is doing the "right thing". In this case, returning a failure status is just not going to work. We've gone too far down the path in order for a failure to cause some failure in the caller. We'll just treat it like virObjectUnlock and if the resource remains locked, well we'll find out relatively soon as the next time someone goes to read/write the list they'll be waiting for an unlock that will never happen because of some coding mistake. But that will save us from perhaps other dire consequences involving corruption. Finally, revert the virObjectLock and virObjectUnlock code to just manage virObjectLockable locks. No worse than what happened before the RWLock was introduced. Leaving the consumers to handle things as they would before - which probably ends up in some sort of awful state leading to memory corruption and a quick death as opposed to abort()'g in the void() function which has been deemed a don't do that type operation for libvirt. The last 2 patches actually have appeared before in various common object series postings, so they could be a v2 or 3 type logic, but I see them more related to this than that, so I present them here again. John Ferlan (7): util: Alter virObjectLockRead to return status util: Introduce and use virObjectLockWrite util: Only have virObjectLock handle virObjectLockable util: Introduce virObjectGetRWLockableObj util: Introduce and use virObjectRWUnlock util: Create common error path for invalid object util: Add safety net of checks to ensure valid object src/conf/virdomainobjlist.c | 81 +++++++++++++--------- src/libvirt_private.syms | 2 + src/util/virobject.c | 160 +++++++++++++++++++++++++++++++++----------- src/util/virobject.h | 10 ++- 4 files changed, 181 insertions(+), 72 deletions(-) -- 2.9.4
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@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 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; } diff --git a/src/util/virobject.h b/src/util/virobject.h index 5985fad..99910e0 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -124,7 +124,7 @@ void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -void +int virObjectLockRead(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
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.
I agree with Dan that this doesn't give any benefit. We should rather consider start using abort() since this is a programming error, not something that depends on an input from user. It should not happen if if it does we have serious issues and abort is a best choice. Pavel
On 07/28/2017 12:56 PM, Pavel Hrdina wrote:
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.
I agree with Dan that this doesn't give any benefit. We should rather consider start using abort() since this is a programming error, not something that depends on an input from user. It should not happen if if it does we have serious issues and abort is a best choice.
Pavel
I'm in the minority, but that's fine. I could also change this patch to be rename virObjectLockRead to be virObjectRWLockRead as suggested later on too. John
On 07/28/2017 08:26 PM, John Ferlan wrote:
On 07/28/2017 12:56 PM, Pavel Hrdina wrote:
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.
I agree with Dan that this doesn't give any benefit. We should rather consider start using abort() since this is a programming error, not something that depends on an input from user. It should not happen if if it does we have serious issues and abort is a best choice.
Pavel
I'm in the minority, but that's fine. I could also change this patch to be rename virObjectLockRead to be virObjectRWLockRead as suggested later on too.
Actually, me choosing virObjectLockRead over virObjectRWLockRead was arbitrary. The reason is that my text editor can offer me completions: virObjectLock virObjectLockWrite virObjectLockRead BTW: Following your reasoning here, it should have been called virObjectLockableLock() instead of virObjectLock() ;-) IOW, I'm failing to see the need for 'RW' in the name you're suggesting. Michal
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@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 :|
[...]
--- 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.
OK - that's fine.
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); }
I'm not as sure for this one though, but I do understand the reasoning especially if @obj == NULL since we'd have a core. Still if @obj is passed in as a non NULL address, I don't believe we're going to get a failure - sure pthread_rwlock_{rdlock|wrlock} or pthread_mutex_lock will fail, but we don't fail on that. Still getting a VIR_WARN somewhere would hopefully help us debug. It's too bad we couldn't have the extra checks be for developer builds only and cause the abort then. In the FWIW department... Even in the first commit ('b545f65d') for virObject{Lock|Unlock}, rudimentary error checking such as !obj and using an @obj with the right class was checked and a VIR_WARN issued. As I was adding a new class for common objects, I made the mistake noted in patch 7, so it seemed to be a good thing to do to add a few more checks along the way and perhaps better entrails. Of course doing that required a multi-step changes... So, commit id '10c2bb2b1' created virObjectGetLockableObj as a way to combine the existing checks. The next steps from my v2 weren't NACK'd, but they weren't ACK'd or discouraged - they just needed some adjustments. The v3 made the adjustments (along with more patches), but never got any reviews. With the addition of RWLockable (commit id '77f4593b') those checks got wiped out in favor of inline code again. I understand why - one API, one place to check, etc. If there's going to be 4 API's, then recreating the original check again is where I was headed, but not it seems like it's not desired even though checks like that have been around from the start. We could proceed with no checks, but before posting patches for that I would like to make sure that's what is really desired given the history and side effect of doing so. Tks - John
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@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; +} + + +/** * 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
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 <phrdina@redhat.com>
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 <phrdina@redhat.com>
Fine again, minority opinion it seems. Still rename from virObjectLockWrite to virObjectRWLockWrite is expect, right (or is that Write) John
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@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 :|
Now that virObjectLockWrite exists to handle the virObjectRWLockable objects, let's restore virObjectLock to only handle virObjectLockable type locks. There still exists the possibility that the input @anyobj isn't a valid object and the resource isn't truly locked, but that also exists before commit id '77f4593b'. This also restores some logic that commit id '77f4593b' removed with respect to a common code path that commit id '10c2bb2b' had introduced as virObjectGetLockableObj. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index c48f88c..f9047a1 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -367,13 +367,28 @@ virObjectRef(void *anyobj) } +static virObjectLockablePtr +virObjectGetLockableObj(void *anyobj) +{ + virObjectPtr obj; + + if (virObjectIsClass(anyobj, virObjectLockableClass)) + return anyobj; + + obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockable or virObjectRWLockable * * Acquire a lock on @anyobj. The lock must be released by - * virObjectUnlock. In case the passed object is instance of - * virObjectRWLockable a write lock is acquired. + * virObjectUnlock. * * The caller is expected to have acquired a reference * on the object before locking it (eg virObjectRef). @@ -383,18 +398,12 @@ virObjectRef(void *anyobj) void virObjectLock(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectLockableClass)) { - virObjectLockablePtr obj = anyobj; - virMutexLock(&obj->lock); - } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { - virObjectRWLockablePtr obj = anyobj; - virRWLockWrite(&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; + + virMutexLock(&obj->lock); } -- 2.9.4
On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
Now that virObjectLockWrite exists to handle the virObjectRWLockable objects, let's restore virObjectLock to only handle virObjectLockable type locks. There still exists the possibility that the input @anyobj isn't a valid object and the resource isn't truly locked, but that also exists before commit id '77f4593b'.
This also restores some logic that commit id '77f4593b' removed with respect to a common code path that commit id '10c2bb2b' had introduced as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Fri, Jul 28, 2017 at 12:38:57PM -0400, John Ferlan wrote:
Now that virObjectLockWrite exists to handle the virObjectRWLockable objects, let's restore virObjectLock to only handle virObjectLockable type locks. There still exists the possibility that the input @anyobj isn't a valid object and the resource isn't truly locked, but that also exists before commit id '77f4593b'.
This also restores some logic that commit id '77f4593b' removed with respect to a common code path that commit id '10c2bb2b' had introduced as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index c48f88c..f9047a1 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -367,13 +367,28 @@ virObjectRef(void *anyobj) }
+static virObjectLockablePtr +virObjectGetLockableObj(void *anyobj) +{ + virObjectPtr obj; + + if (virObjectIsClass(anyobj, virObjectLockableClass)) + return anyobj; + + obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockable or virObjectRWLockable * * Acquire a lock on @anyobj. The lock must be released by - * virObjectUnlock. In case the passed object is instance of - * virObjectRWLockable a write lock is acquired. + * virObjectUnlock. * * The caller is expected to have acquired a reference * on the object before locking it (eg virObjectRef). @@ -383,18 +398,12 @@ virObjectRef(void *anyobj) void virObjectLock(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectLockableClass)) { - virObjectLockablePtr obj = anyobj; - virMutexLock(&obj->lock); - } else if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { - virObjectRWLockablePtr obj = anyobj; - virRWLockWrite(&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; + + virMutexLock(&obj->lock);
Again I'd prefer to see virObjectLockablePtr obj = anyobj; virMutexLock(&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 :|
Introduce a helper to handle the error path more cleanly. The same as virObjectGetLockableObj. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 51 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index f9047a1..0db98c3 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -383,6 +383,22 @@ virObjectGetLockableObj(void *anyobj) } +static virObjectRWLockablePtr +virObjectGetRWLockableObj(void *anyobj) +{ + virObjectPtr obj; + + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) + return anyobj; + + obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockable or virObjectRWLockable @@ -422,18 +438,13 @@ virObjectLock(void *anyobj) int 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; + virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj); + + if (!obj) + return -1; + + virRWLockRead(&obj->lock); + return 0; } @@ -454,18 +465,16 @@ virObjectLockRead(void *anyobj) 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)"); + virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj); + + if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to obtain rwlock for object=%p"), anyobj); + return -1; } - return -1; + + virRWLockWrite(&obj->lock); + return 0; } -- 2.9.4
On Fri, Jul 28, 2017 at 12:38:58PM -0400, John Ferlan wrote:
Introduce a helper to handle the error path more cleanly. The same as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 51 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 21 deletions(-)
Without the return value as for patch 01 and 02 Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
On Fri, Jul 28, 2017 at 12:38:58PM -0400, John Ferlan wrote:
Introduce a helper to handle the error path more cleanly. The same as virObjectGetLockableObj.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 51 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index f9047a1..0db98c3 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -383,6 +383,22 @@ virObjectGetLockableObj(void *anyobj) }
+static virObjectRWLockablePtr +virObjectGetRWLockableObj(void *anyobj) +{ + virObjectPtr obj; + + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) + return anyobj; + + obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockable or virObjectRWLockable @@ -422,18 +438,13 @@ virObjectLock(void *anyobj) int 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; + virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj); + + if (!obj) + return -1; + + virRWLockRead(&obj->lock); + return 0; }
@@ -454,18 +465,16 @@ virObjectLockRead(void *anyobj) 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)"); + virObjectRWLockablePtr obj = virObjectGetRWLockableObj(anyobj); + + if (!obj) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to obtain rwlock for object=%p"), anyobj); + return -1; } - return -1; + + virRWLockWrite(&obj->lock); + return 0; }
I don't really thing this is a good idea for reasons outlined in the replies to earlier patches. 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 :|
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
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>
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
If virObjectIsClass fails "internally" to virobject.c, create a macro to generate the VIR_WARN describing what the problem is. Also improve the checks and message a bit to indicate which was the failure - whether the obj was NULL or just not the right class Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 6f786ce..2cf4743 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,6 +47,17 @@ struct _virClass { virObjectDisposeCallback dispose; }; +#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ + do { \ + virObjectPtr obj = anyobj; \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p (%s) is not a %s instance", \ + anyobj, obj->klass->name, #objclass); \ + } while (0) + + static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; static virClassPtr virObjectRWLockableClass; @@ -370,15 +381,10 @@ virObjectRef(void *anyobj) static virObjectLockablePtr virObjectGetLockableObj(void *anyobj) { - virObjectPtr obj; - if (virObjectIsClass(anyobj, virObjectLockableClass)) return anyobj; - obj = anyobj; - VIR_WARN("Object %p (%s) is not a virObjectLockable instance", - anyobj, obj ? obj->klass->name : "(unknown)"); - + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockable); return NULL; } @@ -386,15 +392,10 @@ virObjectGetLockableObj(void *anyobj) static virObjectRWLockablePtr virObjectGetRWLockableObj(void *anyobj) { - virObjectPtr obj; - if (virObjectIsClass(anyobj, virObjectRWLockableClass)) return anyobj; - obj = anyobj; - VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", - anyobj, obj ? obj->klass->name : "(unknown)"); - + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectRWLockable); return NULL; } -- 2.9.4
On Fri, Jul 28, 2017 at 12:39:00PM -0400, John Ferlan wrote:
If virObjectIsClass fails "internally" to virobject.c, create a macro to generate the VIR_WARN describing what the problem is. Also improve the checks and message a bit to indicate which was the failure - whether the obj was NULL or just not the right class
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
The virObject logic "assumes" that whatever is passed to its API's would be some sort of virObjectPtr; however, if it is not then some really bad things can happen. So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, and virObjectIsClass and the virObject and virObjectLockable class consumers have been well behaved and code well tested. Soon there will be more consumers and one such consumer tripped over this during testing by passing a virHashTablePtr to virObjectIsClass which ends up calling virClassIsDerivedFrom using "obj->klass", which wasn't really a klass object causing one of those bad things to happen. To avoid the future possibility that a non virObject class memory was passed to some virObject* API, this patch adds two new checks - one to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic and the other to ensure obj->u.s.magic doesn't "wrap" some day to 0xCAFF0000 if we ever get that many object classes. The check is also moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would be required on the failure path. It is still left up to the caller to handle the failed API calls just as it would be if it passed a NULL opaque pointer anyobj. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 2cf4743..dd4c39a 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,14 +47,21 @@ struct _virClass { virObjectDisposeCallback dispose; }; +#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) != 0xCAFE0000)) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ - else \ + if (VIR_OBJECT_NOTVALID(obj)) { \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p has a bad magic number %X", \ + obj, obj->u.s.magic); \ + } else { \ VIR_WARN("Object %p (%s) is not a %s instance", \ anyobj, obj->klass->name, #objclass); \ + } \ } while (0) @@ -177,9 +184,14 @@ virClassNew(virClassPtr parent, goto error; klass->parent = parent; + klass->magic = virAtomicIntInc(&magicCounter); + if (klass->magic > 0xCAFEFFFF) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("too many object classes defined")); + goto error; + } if (VIR_STRDUP(klass->name, name) < 0) goto error; - klass->magic = virAtomicIntInc(&magicCounter); klass->objectSize = objectSize; klass->dispose = dispose; @@ -331,7 +343,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false; bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -370,7 +382,7 @@ virObjectRef(void *anyobj) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return NULL; virAtomicIntInc(&obj->u.s.refs); PROBE(OBJECT_REF, "obj=%p", obj); @@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false; return virClassIsDerivedFrom(obj->klass, klass); -- 2.9.4
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
The virObject logic "assumes" that whatever is passed to its API's would be some sort of virObjectPtr; however, if it is not then some really bad things can happen.
So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, and virObjectIsClass and the virObject and virObjectLockable class consumers have been well behaved and code well tested. Soon there will be more consumers and one such consumer tripped over this during testing by passing a virHashTablePtr to virObjectIsClass which ends up calling virClassIsDerivedFrom using "obj->klass", which wasn't really a klass object causing one of those bad things to happen.
To avoid the future possibility that a non virObject class memory was passed to some virObject* API, this patch adds two new checks - one to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic and the other to ensure obj->u.s.magic doesn't "wrap" some day to 0xCAFF0000 if we ever get that many object classes. The check is also moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would be required on the failure path.
This doesn't add any safeguard and for most virObject(Ref|Unref) calls we don't check the return value. This is basically a programming error as well and we should consider start using abort().
It is still left up to the caller to handle the failed API calls just as it would be if it passed a NULL opaque pointer anyobj.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 2cf4743..dd4c39a 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,14 +47,21 @@ struct _virClass { virObjectDisposeCallback dispose; };
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) != 0xCAFE0000))
This is not correct, it should be: ((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)
+ #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ - else \ + if (VIR_OBJECT_NOTVALID(obj)) { \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p has a bad magic number %X", \ + obj, obj->u.s.magic); \ + } else { \ VIR_WARN("Object %p (%s) is not a %s instance", \ anyobj, obj->klass->name, #objclass); \ + } \ } while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent, goto error;
klass->parent = parent; + klass->magic = virAtomicIntInc(&magicCounter); + if (klass->magic > 0xCAFEFFFF) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("too many object classes defined")); + goto error; + }
This will probably never happen :). Pavel
On 07/28/2017 01:19 PM, Pavel Hrdina wrote:
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
The virObject logic "assumes" that whatever is passed to its API's would be some sort of virObjectPtr; however, if it is not then some really bad things can happen.
So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, and virObjectIsClass and the virObject and virObjectLockable class consumers have been well behaved and code well tested. Soon there will be more consumers and one such consumer tripped over this during testing by passing a virHashTablePtr to virObjectIsClass which ends up calling virClassIsDerivedFrom using "obj->klass", which wasn't really a klass object causing one of those bad things to happen.
To avoid the future possibility that a non virObject class memory was passed to some virObject* API, this patch adds two new checks - one to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic and the other to ensure obj->u.s.magic doesn't "wrap" some day to 0xCAFF0000 if we ever get that many object classes. The check is also moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would be required on the failure path.
This doesn't add any safeguard and for most virObject(Ref|Unref) calls we don't check the return value. This is basically a programming error as well and we should consider start using abort().
And yes, this is the programming error - it was awfully stupid, but IIRC it also didn't "jump out" right away what the problem is/was. It was far worse for *Ref/Unref, but lock was interesting too insomuch as I wouldn't get the lock so perhaps two threads would make a change at the same time and we may never know unless we check lock status.
It is still left up to the caller to handle the failed API calls just as it would be if it passed a NULL opaque pointer anyobj.
And of course this was my escape clause because of void Lock's.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 2cf4743..dd4c39a 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,14 +47,21 @@ struct _virClass { virObjectDisposeCallback dispose; };
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) != 0xCAFE0000))
This is not correct, it should be:
((obj->u.s.magic & 0xFFFF0000) != 0xCAFE0000)
Oh right and I see it was a straight cut-n-paste from Dan's reply too: https://www.redhat.com/archives/libvir-list/2017-May/msg01211.html
+ #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ - else \ + if (VIR_OBJECT_NOTVALID(obj)) { \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p has a bad magic number %X", \ + obj, obj->u.s.magic); \ + } else { \ VIR_WARN("Object %p (%s) is not a %s instance", \ anyobj, obj->klass->name, #objclass); \ + } \ } while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent, goto error;
klass->parent = parent; + klass->magic = virAtomicIntInc(&magicCounter); + if (klass->magic > 0xCAFEFFFF) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("too many object classes defined")); + goto error; + }
This will probably never happen :).
Pavel
Agreed, but if it does happen then I'd be the last to touch right... So I'd get the blame. In any case, another one of those Dan suggestions: https://www.redhat.com/archives/libvir-list/2017-May/msg01221.html If this is not desired - that's fine I can drop it. Cannot say I didn't try though. Thanks for the quick review! With a freeze on at least this may give time for others to chime in with thoughts as well. John
On Fri, Jul 28, 2017 at 12:39:01PM -0400, John Ferlan wrote:
The virObject logic "assumes" that whatever is passed to its API's would be some sort of virObjectPtr; however, if it is not then some really bad things can happen.
So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, and virObjectIsClass and the virObject and virObjectLockable class consumers have been well behaved and code well tested. Soon there will be more consumers and one such consumer tripped over this during testing by passing a virHashTablePtr to virObjectIsClass which ends up calling virClassIsDerivedFrom using "obj->klass", which wasn't really a klass object causing one of those bad things to happen.
To avoid the future possibility that a non virObject class memory was passed to some virObject* API, this patch adds two new checks - one to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic and the other to ensure obj->u.s.magic doesn't "wrap" some day to 0xCAFF0000 if we ever get that many object classes. The check is also moved before the name VIR_STRDUP to avoid the extra VIR_FREE that would be required on the failure path.
It is still left up to the caller to handle the failed API calls just as it would be if it passed a NULL opaque pointer anyobj.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 2cf4743..dd4c39a 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,14 +47,21 @@ struct _virClass { virObjectDisposeCallback dispose; };
+#define VIR_OBJECT_NOTVALID(obj) (!obj || ((obj->u.s.magic & 0xCAFE0000) != 0xCAFE0000)) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ - VIR_WARN("Object cannot be NULL"); \ - else \ + if (VIR_OBJECT_NOTVALID(obj)) { \ + if (!obj) \ + VIR_WARN("Object cannot be NULL"); \ + else \ + VIR_WARN("Object %p has a bad magic number %X", \ + obj, obj->u.s.magic); \ + } else { \ VIR_WARN("Object %p (%s) is not a %s instance", \ anyobj, obj->klass->name, #objclass); \ + } \ } while (0)
@@ -177,9 +184,14 @@ virClassNew(virClassPtr parent, goto error;
klass->parent = parent; + klass->magic = virAtomicIntInc(&magicCounter); + if (klass->magic > 0xCAFEFFFF) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("too many object classes defined")); + goto error; + } if (VIR_STRDUP(klass->name, name) < 0) goto error; - klass->magic = virAtomicIntInc(&magicCounter); klass->objectSize = objectSize; klass->dispose = dispose;
@@ -331,7 +343,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj;
- if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -370,7 +382,7 @@ virObjectRef(void *anyobj) { virObjectPtr obj = anyobj;
- if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return NULL; virAtomicIntInc(&obj->u.s.refs); PROBE(OBJECT_REF, "obj=%p", obj); @@ -532,7 +544,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
I really don't think these changes are a positive move. If you have code that is passing in something that is not a valid object, then silently doing nothing in virObjectRef / virObjectIsClass is not going to make the code any more correct. In fact you're turning something that could be an immediate crash (and thus easy to diagnose) into something that could be silent bad behaviour, which is much harder to diagnose, or cause a crash much further away from the original root bug. 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 :|
[...]
I really don't think these changes are a positive move.
If you have code that is passing in something that is not a valid object, then silently doing nothing in virObjectRef / virObjectIsClass is not going to make the code any more correct. In fact you're turning something that could be an immediate crash (and thus easy to diagnose) into something that could be silent bad behaviour, which is much harder to diagnose, or cause a crash much further away from the original root bug.
Consider the longevity of the patch being on list - I cannot remember all the details, although the commit message does help a bit. I do recall looking at the code and thinking incorrect usage could lead down the road of bad things. For Ref/Unref all that has been checked is !obj and we Incr/Decr a location. For Lock/Unlock as described in my 1/7 response class checking is/was added. Adding in object validity for Ref/Unref at least avoids rogue corruption which is really hard to diagnose in favor of leaving an entrail. Even without the additional check, the @obj someone may have thought they were incr/decr the refcnt isn't going to happen. Still, it just seems better in the event that we don't have a real @obj to at least message that in the hopes that someone trips across it. Similarly for Lock/Unlock same thing. IMO: The patches aren't making it harder to diagnose a problem - they're helping and they're not altering the value of some field for a valid @obj address. But if that's not desired, then fine - at least attempt was made and the feedback has been provided. Tks - John
participants (4)
-
Daniel P. Berrange -
John Ferlan -
Michal Privoznik -
Pavel Hrdina