[libvirt] [PATCH v2 0/8] Some virObjectRW* adjustments
v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html Differences from v1: * Use the names virObjectRWLockRead, virObjectRWLockWrite and virObjectRWUnlock * Instead of an 'int' return, make the virObjectRWLock{Read|Write} be void returns just like virObject{Lock|Unlock} * Separate out the magic number check for the virObjectIsClass consumers into its own patch and describe the reasons for usage. * Apply the same magic number check to virObject{Ref|Unref} separately. BTW: This looks and works eally nice with what I have for common objects... John Ferlan (8): util: Rename virObjectLockRead to virObjectRWLockRead util: Introduce and use virObjectRWLockWrite util: Only have virObjectLock handle virObjectLockable util: Introduce virObjectGetRWLockableObj util: Introduce and use virObjectRWUnlock util: Create common error path for invalid object util: Add magic number check for object validity util: Add object checking for virObject{Ref|Unref} src/conf/virdomainobjlist.c | 62 ++++++++-------- src/libvirt_private.syms | 4 +- src/util/virobject.c | 169 +++++++++++++++++++++++++++++++++----------- src/util/virobject.h | 10 ++- 4 files changed, 169 insertions(+), 76 deletions(-) -- 2.9.4
Since the class it represents is based on virObjectRWLockableClass and in order to make sure we diffentiate lest anyone somehow believes they could use virObjectLockRead for a virObjectLockableClass, let's rename the API to use the RW in the name. Besides the RW locks refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the other locks refer to pthread_mutex_{init|lock|unlock|destroy}. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 18 +++++++++--------- src/libvirt_private.syms | 2 +- src/util/virobject.c | 11 ++++++++--- src/util/virobject.h | 2 +- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 1d027a4..9bc6603 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, bool ref) { virDomainObjPtr obj; - virObjectLockRead(doms); + virObjectRWLockRead(doms); obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL); if (ref) { virObjectRef(obj); @@ -160,7 +160,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj; - virObjectLockRead(doms); + virObjectRWLockRead(doms); virUUIDFormat(uuid, uuidstr); obj = virHashLookup(doms->objs, uuidstr); @@ -204,7 +204,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, { virDomainObjPtr obj; - virObjectLockRead(doms); + virObjectRWLockRead(doms); obj = virHashLookup(doms->objsName, name); virObjectRef(obj); virObjectUnlock(doms); @@ -653,7 +653,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms, virConnectPtr conn) { struct virDomainObjListData data = { filter, conn, active, 0 }; - virObjectLockRead(doms); + virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListCount, &data); virObjectUnlock(doms); return data.count; @@ -697,7 +697,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms, { struct virDomainIDData data = { filter, conn, 0, maxids, ids }; - virObjectLockRead(doms); + virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data); virObjectUnlock(doms); return data.numids; @@ -751,7 +751,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms, struct virDomainNameData data = { filter, conn, 0, 0, maxnames, names }; size_t i; - virObjectLockRead(doms); + virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); virObjectUnlock(doms); if (data.oom) { @@ -792,7 +792,7 @@ virDomainObjListForEach(virDomainObjListPtr doms, struct virDomainListIterData data = { callback, opaque, 0, }; - virObjectLockRead(doms); + virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListHelper, &data); virObjectUnlock(doms); return data.ret; @@ -925,7 +925,7 @@ virDomainObjListCollect(virDomainObjListPtr domlist, { struct virDomainListData data = { NULL, 0 }; - virObjectLockRead(domlist); + virObjectRWLockRead(domlist); sa_assert(domlist->objs); if (VIR_ALLOC_N(data.vms, virHashSize(domlist->objs)) < 0) { virObjectUnlock(domlist); @@ -962,7 +962,7 @@ virDomainObjListConvert(virDomainObjListPtr domlist, *nvms = 0; *vms = NULL; - virObjectLockRead(domlist); + virObjectRWLockRead(domlist); for (i = 0; i < ndoms; i++) { virDomainPtr dom = doms[i]; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 37b815c..99302d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2305,10 +2305,10 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; -virObjectLockRead; virObjectNew; virObjectRef; virObjectRWLockableNew; +virObjectRWLockRead; virObjectUnlock; virObjectUnref; diff --git a/src/util/virobject.c b/src/util/virobject.c index b1bb378..b97f251 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -399,7 +399,7 @@ virObjectLock(void *anyobj) /** - * virObjectLockRead: + * virObjectRWLockRead: * @anyobj: any instance of virObjectRWLockable * * Acquire a read lock on @anyobj. The lock must be @@ -409,9 +409,14 @@ virObjectLock(void *anyobj) * on the object before locking it (eg virObjectRef). * The object must be unlocked before releasing this * reference. + * + * NB: It's possible to return without the lock if + * @anyobj was invalid - this has been considered + * a programming error rather than something that + * should be checked. */ void -virObjectLockRead(void *anyobj) +virObjectRWLockRead(void *anyobj) { if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { virObjectRWLockablePtr obj = anyobj; @@ -429,7 +434,7 @@ virObjectLockRead(void *anyobj) * @anyobj: any instance of virObjectLockable or virObjectRWLockable * * Release a lock on @anyobj. The lock must have been acquired by - * virObjectLock or virObjectLockRead. + * virObjectLock or virObjectRWLockRead. */ void virObjectUnlock(void *anyobj) diff --git a/src/util/virobject.h b/src/util/virobject.h index 5985fad..e7ca983 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -125,7 +125,7 @@ virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); void -virObjectLockRead(void *lockableobj) +virObjectRWLockRead(void *lockableobj) ATTRIBUTE_NONNULL(1); void -- 2.9.4
On 08/01/2017 02:05 AM, John Ferlan wrote:
Since the class it represents is based on virObjectRWLockableClass and in order to make sure we diffentiate lest anyone somehow
^^ couple of typos
believes they could use virObjectLockRead for a virObjectLockableClass, let's rename the API to use the RW in the name. Besides the RW locks refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the other locks refer to pthread_mutex_{init|lock|unlock|destroy}.
Firstly, this is just an internal implementation. We often rename APIs for us to use. Secondly, this is because pthreads are C API with no 'object' reference. So they have to have two unlock functions for two different objects.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 18 +++++++++--------- src/libvirt_private.syms | 2 +- src/util/virobject.c | 11 ++++++++--- src/util/virobject.h | 2 +- 4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 1d027a4..9bc6603 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, bool ref) { virDomainObjPtr obj; - virObjectLockRead(doms); + virObjectRWLockRead(doms);
If we are going to do this (which I'm no fan of), then we should also turn virObjectLock() into virObjectLockableLock(). For the consistency sake. Moreover, as I stated in discussion to v1 (not sure if you've read it before sending this series), I quite like that I'm able to type virObjectLock, hit shortcut for bringing up completion list and chose 'virObjectLock', 'virObjectLockRead', (or even) 'virObjectLockWrite'. With this patch I'm no longer able to do that so easily. Michal
On 08/01/2017 09:24 AM, Michal Privoznik wrote:
On 08/01/2017 02:05 AM, John Ferlan wrote:
Since the class it represents is based on virObjectRWLockableClass and in order to make sure we diffentiate lest anyone somehow
^^ couple of typos
Just differentiate from my dictionary. 'lest' is someone colloquial - I can alter it though.
believes they could use virObjectLockRead for a virObjectLockableClass, let's rename the API to use the RW in the name. Besides the RW locks refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the other locks refer to pthread_mutex_{init|lock|unlock|destroy}.
Firstly, this is just an internal implementation. We often rename APIs for us to use. Secondly, this is because pthreads are C API with no 'object' reference. So they have to have two unlock functions for two different objects.
Well function naming guidelines weren't in place when virObjectLock (and Unlock) were added, but they are now.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 18 +++++++++--------- src/libvirt_private.syms | 2 +- src/util/virobject.c | 11 ++++++++--- src/util/virobject.h | 2 +- 4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 1d027a4..9bc6603 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, bool ref) { virDomainObjPtr obj; - virObjectLockRead(doms); + virObjectRWLockRead(doms);
If we are going to do this (which I'm no fan of), then we should also turn virObjectLock() into virObjectLockableLock(). For the consistency sake. Moreover, as I stated in discussion to v1 (not sure if you've read it before sending this series), I quite like that I'm able to type virObjectLock, hit shortcut for bringing up completion list and chose 'virObjectLock', 'virObjectLockRead', (or even) 'virObjectLockWrite'. With this patch I'm no longer able to do that so easily.
Michal
I read the response and the others... I'm torn between RWLockRead and just LockRead. I really don't care either way. I went with RW for the stated reason though - fear that someone like you sees virObjectLockRead (or worse virObjectLockWrite) and believes that is what they want. If they are not operating on an RWLockableObject, then they really don't get the lock and because we've decided to not error out in that case, they don't have the safety they thought they had. Maybe I'm wrong, but I have to present that argument. As for altering virObjectLock to virObjectLockableLock - that ship sailed long ago. I would say it would be virObjectMutexLock though to be more precise, but using virObjectLock as a shorthand since there was only one locking subsystem available is understandable. Time has brought forth some new options and now we have to adjust the new code to fit the more recent models. The old code could be adjusted, but there's far too many places that need change and no one wants that insanely impossible task. I suppose I could also see just reason to go with virObjectLockRWRdLock and virObjectLockRWWrLock (and virObjectUnlockRW). I haven't trained my editor to choose API names for me. Not sure there's a perfect solution for this - perhaps multiple opinions though. I suppose all that really matters is we decide either: 1. Leave things as they are - completely 2. Alter the naming scheme in some way I can live with #1 even though I'm concerned about mis-use. Also, I thought using overloaded functions was something that long ago was decided to be less desirable. That is the Lock and Unlock operate on two different object types based on something stored in the object instead of two separate API's. The call is to two very different lower level API's as well that cannot be used interchangeably. While IIUC the goal would be to some day change all virObjectLock()'s to be either LockRead or LockWrite and do away with virObjectLock and any sort of reference to virMutexLock's and that's a noble goal. However, I would also think it could be awhile before that's realizable. So yes, it's a conundrum and I can be talked into dropping this series. Although I do still see value in the "magic number check" prior to using a non NULL @obj (a/k/a @anyobj). John FWIW: As it relates to common object - since I've chosen to use a LockableLock and RWLockable as "parent"'s to my new object children, those conditions that check virObjectIsClass(anyobj,... get a bit more ugly looking or are replaced by something that can check the parent or the parent's parent (letting someone else figure out 3 levels of a family tree).
On 08/01/2017 05:36 PM, John Ferlan wrote:
On 08/01/2017 09:24 AM, Michal Privoznik wrote:
On 08/01/2017 02:05 AM, John Ferlan wrote:
Since the class it represents is based on virObjectRWLockableClass and in order to make sure we diffentiate lest anyone somehow
^^ couple of typos
Just differentiate from my dictionary.
'lest' is someone colloquial - I can alter it though.
Ah, okay. I'm no native speaker. Learned something new :-)
believes they could use virObjectLockRead for a virObjectLockableClass, let's rename the API to use the RW in the name. Besides the RW locks refer to pthread_rwlock_{init|rdlock|wrlock|unlock|destroy} while the other locks refer to pthread_mutex_{init|lock|unlock|destroy}.
Firstly, this is just an internal implementation. We often rename APIs for us to use. Secondly, this is because pthreads are C API with no 'object' reference. So they have to have two unlock functions for two different objects.
Well function naming guidelines weren't in place when virObjectLock (and Unlock) were added, but they are now.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 18 +++++++++--------- src/libvirt_private.syms | 2 +- src/util/virobject.c | 11 ++++++++--- src/util/virobject.h | 2 +- 4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 1d027a4..9bc6603 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -118,7 +118,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, bool ref) { virDomainObjPtr obj; - virObjectLockRead(doms); + virObjectRWLockRead(doms);
If we are going to do this (which I'm no fan of), then we should also turn virObjectLock() into virObjectLockableLock(). For the consistency sake. Moreover, as I stated in discussion to v1 (not sure if you've read it before sending this series), I quite like that I'm able to type virObjectLock, hit shortcut for bringing up completion list and chose 'virObjectLock', 'virObjectLockRead', (or even) 'virObjectLockWrite'. With this patch I'm no longer able to do that so easily.
Michal
I read the response and the others... I'm torn between RWLockRead and just LockRead. I really don't care either way. I went with RW for the stated reason though - fear that someone like you sees virObjectLockRead (or worse virObjectLockWrite) and believes that is what they want.
If they are not operating on an RWLockableObject, then they really don't get the lock and because we've decided to not error out in that case, they don't have the safety they thought they had.
Well, true. But aren't we trying to fix something that is no issue? I mean, have somebody ran into such issue? But truth is I no longer care that much.
Maybe I'm wrong, but I have to present that argument.
As for altering virObjectLock to virObjectLockableLock - that ship sailed long ago. I would say it would be virObjectMutexLock though to be more precise, but using virObjectLock as a shorthand since there was only one locking subsystem available is understandable. Time has brought forth some new options and now we have to adjust the new code to fit the more recent models. The old code could be adjusted, but there's far too many places that need change and no one wants that insanely impossible task.
I suppose I could also see just reason to go with virObjectLockRWRdLock and virObjectLockRWWrLock (and virObjectUnlockRW).
I haven't trained my editor to choose API names for me.
You should, esp. with such long function names :-)
Not sure there's a perfect solution for this - perhaps multiple opinions though. I suppose all that really matters is we decide either:
1. Leave things as they are - completely 2. Alter the naming scheme in some way
I can live with #1 even though I'm concerned about mis-use. Also, I thought using overloaded functions was something that long ago was decided to be less desirable. That is the Lock and Unlock operate on two different object types based on something stored in the object instead of two separate API's. The call is to two very different lower level API's as well that cannot be used interchangeably.
While IIUC the goal would be to some day change all virObjectLock()'s to be either LockRead or LockWrite and do away with virObjectLock and any sort of reference to virMutexLock's and that's a noble goal. However, I would also think it could be awhile before that's realizable. So yes, it's a conundrum and I can be talked into dropping this series. Although I do still see value in the "magic number check" prior to using a non NULL @obj (a/k/a @anyobj).
I don't think that we want to replace all mutexes with rwlocks. Firstly, rwlocks come with some overhead over mutexes, secondly there are some places where we do writes in all critical sections. For instance I don't think that event loop's mutex is a candidate for rwlock. On the other hand, it's not a virObject... Michal
Instead of making virObjectLock be the entry point for two different types of locks, let's create a virObjectRWLockWrite API which will only handle the virObjectRWLockableClass objects. Use the new virObjectRWLockWrite for the virdomainobjlist code in order to handle the Add, Remove, Rename, and Load operations that need to be very synchronous. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 8 ++++---- src/libvirt_private.syms | 1 + src/util/virobject.c | 33 ++++++++++++++++++++++++++++++++- src/util/virobject.h | 4 ++++ 4 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 9bc6603..573032f 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -327,7 +327,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, { virDomainObjPtr ret; - virObjectLock(doms); + virObjectRWLockWrite(doms); ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef); virObjectUnlock(doms); return ret; @@ -349,7 +349,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms, virObjectRef(dom); virObjectUnlock(dom); - virObjectLock(doms); + virObjectRWLockWrite(doms); virObjectLock(dom); virHashRemoveEntry(doms->objs, uuidstr); virHashRemoveEntry(doms->objsName, dom->def->name); @@ -394,7 +394,7 @@ virDomainObjListRename(virDomainObjListPtr doms, * hold a lock on dom but not refcount it. */ virObjectRef(dom); virObjectUnlock(dom); - virObjectLock(doms); + virObjectRWLockWrite(doms); virObjectLock(dom); virObjectUnref(dom); @@ -573,7 +573,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, if ((rc = virDirOpenIfExists(&dir, configDir)) <= 0) return rc; - virObjectLock(doms); + virObjectRWLockWrite(doms); while ((ret = virDirRead(dir, &entry, configDir)) > 0) { virDomainObjPtr dom; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99302d2..7f2f6d6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2309,6 +2309,7 @@ virObjectNew; virObjectRef; virObjectRWLockableNew; virObjectRWLockRead; +virObjectRWLockWrite; virObjectUnlock; virObjectUnref; diff --git a/src/util/virobject.c b/src/util/virobject.c index b97f251..f49af62 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -430,11 +430,42 @@ virObjectRWLockRead(void *anyobj) /** + * virObjectRWLockWrite: + * @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. + * + * NB: It's possible to return without the lock if + * @anyobj was invalid - this has been considered + * a programming error rather than something that + * should be checked. + */ +void +virObjectRWLockWrite(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { + virObjectRWLockablePtr obj = anyobj; + virRWLockWrite(&obj->lock); + } else { + virObjectPtr obj = anyobj; + VIR_WARN("Object %p (%s) is not a virObjectRWLockable instance", + anyobj, obj ? obj->klass->name : "(unknown)"); + } +} + + +/** * virObjectUnlock: * @anyobj: any instance of virObjectLockable or virObjectRWLockable * * Release a lock on @anyobj. The lock must have been acquired by - * virObjectLock or virObjectRWLockRead. + * virObjectLock, virObjectRWLockRead, or virObjectRWLockWrite. */ void virObjectUnlock(void *anyobj) diff --git a/src/util/virobject.h b/src/util/virobject.h index e7ca983..24ee6dd 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -129,6 +129,10 @@ virObjectRWLockRead(void *lockableobj) ATTRIBUTE_NONNULL(1); void +virObjectRWLockWrite(void *lockableobj) + ATTRIBUTE_NONNULL(1); + +void virObjectUnlock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Now that virObjectRWLockWrite exists to handle the virObjectRWLockable objects, let's restore virObjectLock to only handle virObjectLockable class 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. This code path merely does the same checks as the original virObjectLock commit 'b545f65d', but in callable/reusable helper to ensure the @obj at least has some validity before using. 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 f49af62..4903393 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 08/01/2017 02:05 AM, John Ferlan wrote:
Now that virObjectRWLockWrite exists to handle the virObjectRWLockable objects, let's restore virObjectLock to only handle virObjectLockable class 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. This code path merely does the same checks as the original virObjectLock commit 'b545f65d', but in callable/reusable helper to ensure the @obj at least has some validity before using.
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 f49af62..4903393 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)");
If we're really worried about this (and don't want to abort()), we might consider raising this to VIR_ERROR. I'm a programmer, I don't care about warnings O:-) Michal
Introduce a helper to handle the error path more cleanly. The same as virObjectGetLockableObj in order to essentially follow the original logic of commit 'b545f65d' to ensure that the input argument at least has some validity before using. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 4903393..c1e4474 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 @@ -427,14 +443,12 @@ virObjectLock(void *anyobj) void virObjectRWLockRead(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { - virObjectRWLockablePtr obj = anyobj; - virRWLockRead(&obj->lock); - } 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) + return; + + virRWLockRead(&obj->lock); } @@ -458,14 +472,12 @@ virObjectRWLockRead(void *anyobj) void virObjectRWLockWrite(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectRWLockableClass)) { - virObjectRWLockablePtr obj = anyobj; - virRWLockWrite(&obj->lock); - } 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) + return; + + virRWLockWrite(&obj->lock); } -- 2.9.4
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. Similar to the RWLockRead and RWLockWrite, use the virObjectGetRWLockableObj helper. This restores the virObjectUnlock code to using the virObjectGetLockableObj. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virdomainobjlist.c | 36 ++++++++++++++++++------------------ src/libvirt_private.syms | 1 + src/util/virobject.c | 45 +++++++++++++++++++++++++++++---------------- src/util/virobject.h | 4 ++++ 4 files changed, 52 insertions(+), 34 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 573032f..d874133 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -122,7 +122,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL); if (ref) { virObjectRef(obj); - virObjectUnlock(doms); + virObjectRWUnlock(doms); } if (obj) { virObjectLock(obj); @@ -134,7 +134,7 @@ virDomainObjListFindByIDInternal(virDomainObjListPtr doms, } } if (!ref) - virObjectUnlock(doms); + virObjectRWUnlock(doms); return obj; } @@ -166,7 +166,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, obj = virHashLookup(doms->objs, uuidstr); if (ref) { virObjectRef(obj); - virObjectUnlock(doms); + virObjectRWUnlock(doms); } if (obj) { virObjectLock(obj); @@ -178,7 +178,7 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, } } if (!ref) - virObjectUnlock(doms); + virObjectRWUnlock(doms); return obj; } @@ -207,7 +207,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, virObjectRWLockRead(doms); obj = virHashLookup(doms->objsName, name); virObjectRef(obj); - virObjectUnlock(doms); + virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); if (obj->removing) { @@ -329,7 +329,7 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, virObjectRWLockWrite(doms); ret = virDomainObjListAddLocked(doms, def, xmlopt, flags, oldDef); - virObjectUnlock(doms); + virObjectRWUnlock(doms); return ret; } @@ -355,7 +355,7 @@ void virDomainObjListRemove(virDomainObjListPtr doms, virHashRemoveEntry(doms->objsName, dom->def->name); virObjectUnlock(dom); virObjectUnref(dom); - virObjectUnlock(doms); + virObjectRWUnlock(doms); } @@ -420,7 +420,7 @@ virDomainObjListRename(virDomainObjListPtr doms, ret = 0; cleanup: - virObjectUnlock(doms); + virObjectRWUnlock(doms); VIR_FREE(old_name); return ret; } @@ -609,7 +609,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, } VIR_DIR_CLOSE(dir); - virObjectUnlock(doms); + virObjectRWUnlock(doms); return ret; } @@ -655,7 +655,7 @@ virDomainObjListNumOfDomains(virDomainObjListPtr doms, struct virDomainObjListData data = { filter, conn, active, 0 }; virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListCount, &data); - virObjectUnlock(doms); + virObjectRWUnlock(doms); return data.count; } @@ -699,7 +699,7 @@ virDomainObjListGetActiveIDs(virDomainObjListPtr doms, 0, maxids, ids }; virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListCopyActiveIDs, &data); - virObjectUnlock(doms); + virObjectRWUnlock(doms); return data.numids; } @@ -753,7 +753,7 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms, size_t i; virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); - virObjectUnlock(doms); + virObjectRWUnlock(doms); if (data.oom) { for (i = 0; i < data.numnames; i++) VIR_FREE(data.names[i]); @@ -794,7 +794,7 @@ virDomainObjListForEach(virDomainObjListPtr doms, }; virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListHelper, &data); - virObjectUnlock(doms); + virObjectRWUnlock(doms); return data.ret; } @@ -928,12 +928,12 @@ virDomainObjListCollect(virDomainObjListPtr domlist, virObjectRWLockRead(domlist); 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); @@ -972,7 +972,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); @@ -982,12 +982,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 7f2f6d6..97aaf37 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2310,6 +2310,7 @@ virObjectRef; virObjectRWLockableNew; virObjectRWLockRead; virObjectRWLockWrite; +virObjectRWUnlock; virObjectUnlock; virObjectUnref; diff --git a/src/util/virobject.c b/src/util/virobject.c index c1e4474..85e5a53 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -428,7 +428,7 @@ virObjectLock(void *anyobj) * @anyobj: any instance of virObjectRWLockable * * Acquire a read lock on @anyobj. The lock must be - * released by virObjectUnlock. + * released by virObjectRWUnlock. * * The caller is expected to have acquired a reference * on the object before locking it (eg virObjectRef). @@ -457,7 +457,7 @@ virObjectRWLockRead(void *anyobj) * @anyobj: any instance of virObjectRWLockable * * Acquire a write lock on @anyobj. The lock must be - * released by virObjectUnlock. + * released by virObjectRWUnlock. * * The caller is expected to have acquired a reference * on the object before locking it (eg virObjectRef). @@ -483,26 +483,39 @@ virObjectRWLockWrite(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, virObjectRWLockRead, or virObjectRWLockWrite. + * 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 + * virObjectRWLockRead or virObjectRWLockWrite. + */ +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 24ee6dd..ac6cf22 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
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 85e5a53..af3f252 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
The virObjectIsClass API has only ever checked object validity based on if the @obj is not NULL and it was derived from some class. While this has worked well in general, there is one additional check that could be made prior to calling virClassIsDerivedFrom which loops through the classes checking the magic number against the klass expected magic number. If by chance a non virObject is passed, rather than assuming the void * @obj is a _virObject and thus offsetting to obj->klass, obj->magic, and obj->parent, let's check that the void * @obj has at least the "base part" of the magic number in the right place and generate a more specific VIR_WARN message if not. There are many consumers to virObjectIsClass, include the locking primitives virObject{Lock|Unlock}, virObjectRWLock{Read|Write}, and virObjectRWUnlock. For those callers, the locking call will not fail, but it also will not attempt a virMutex* call which will "most likely" fail since the &obj->lock is used. In order to avoid some possible future wrap on the 0xCAFExxxx value, add a check during initialization that some new class won't cause the wrap. Should be good for a few years at least! 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 | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index af3f252..54d78b0 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 & 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; + } if (VIR_STRDUP(klass->name, name) < 0) goto error; - klass->magic = virAtomicIntInc(&magicCounter); klass->objectSize = objectSize; klass->dispose = dispose; @@ -535,7 +547,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
Rather than assuming that what's passed to virObject{Ref|Unref} would be a virObjectPtr as long as it's not NULL, let's do the similar checks virObjectIsClass in order to prevent a possible increment or decrement to some field at the obj->u.s.refs offset. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 54d78b0..cfa821c 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -343,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); @@ -382,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); -- 2.9.4
On 08/01/2017 02:05 AM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html
Differences from v1:
* Use the names virObjectRWLockRead, virObjectRWLockWrite and virObjectRWUnlock
* Instead of an 'int' return, make the virObjectRWLock{Read|Write} be void returns just like virObject{Lock|Unlock}
* Separate out the magic number check for the virObjectIsClass consumers into its own patch and describe the reasons for usage.
* Apply the same magic number check to virObject{Ref|Unref} separately.
BTW: This looks and works eally nice with what I have for common objects...
John Ferlan (8): util: Rename virObjectLockRead to virObjectRWLockRead util: Introduce and use virObjectRWLockWrite util: Only have virObjectLock handle virObjectLockable util: Introduce virObjectGetRWLockableObj util: Introduce and use virObjectRWUnlock util: Create common error path for invalid object util: Add magic number check for object validity util: Add object checking for virObject{Ref|Unref}
src/conf/virdomainobjlist.c | 62 ++++++++-------- src/libvirt_private.syms | 4 +- src/util/virobject.c | 169 +++++++++++++++++++++++++++++++++----------- src/util/virobject.h | 10 ++- 4 files changed, 169 insertions(+), 76 deletions(-)
Okay, I've ran some local tests and indeed no tool showed any misbehaviour when my test binary was mutex-locking an RW lock or rwlocking a mutex. While I still believe that us, reviewers have to be careful around locks anyway, rename to virObjectRWLock* can help us remember if we forget. ACK series. Michal
On 08/03/2017 11:34 AM, Michal Privoznik wrote:
On 08/01/2017 02:05 AM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2017-July/msg01313.html
Differences from v1:
* Use the names virObjectRWLockRead, virObjectRWLockWrite and virObjectRWUnlock
* Instead of an 'int' return, make the virObjectRWLock{Read|Write} be void returns just like virObject{Lock|Unlock}
* Separate out the magic number check for the virObjectIsClass consumers into its own patch and describe the reasons for usage.
* Apply the same magic number check to virObject{Ref|Unref} separately.
BTW: This looks and works eally nice with what I have for common objects...
John Ferlan (8): util: Rename virObjectLockRead to virObjectRWLockRead util: Introduce and use virObjectRWLockWrite util: Only have virObjectLock handle virObjectLockable util: Introduce virObjectGetRWLockableObj util: Introduce and use virObjectRWUnlock util: Create common error path for invalid object util: Add magic number check for object validity util: Add object checking for virObject{Ref|Unref}
src/conf/virdomainobjlist.c | 62 ++++++++-------- src/libvirt_private.syms | 4 +- src/util/virobject.c | 169 +++++++++++++++++++++++++++++++++----------- src/util/virobject.h | 10 ++- 4 files changed, 169 insertions(+), 76 deletions(-)
Okay, I've ran some local tests and indeed no tool showed any misbehaviour when my test binary was mutex-locking an RW lock or rwlocking a mutex. While I still believe that us, reviewers have to be careful around locks anyway, rename to virObjectRWLock* can help us remember if we forget.
ACK series.
Michal
Thanks - I'll hold off on pushing though... I'm fine with using VIR_ERROR instead of VIR_WARN too... In any case, I'm away next week and wouldn't want to leave that kind of present for anyone to revert! Besides, I'm assuming there may be other opinions and there isn't a "rush" to get this in as nothing is broken yet... John
participants (2)
-
John Ferlan -
Michal Privoznik