[libvirt] [PATCH 0/9] virObject adjustments for common object
Originally posted in part as an RFC: https://www.redhat.com/archives/libvir-list/2017-April/msg00321.html Obviously not a 3.4.0 discussion, but I'm trying to get ahead of the curve for 3.5.0 and posting this now as I'm "close enough" to at least convert secrets, nwfilters, nodedevs, and interfaces to use a more common object methodolgy to add/store, fetch/find, search, list, etc. the objects in something other than a linear forward linked list. Converting the storage pool, storage volume, and network is taking longer than hoped since there are so many patches to get from pointA to pointB. So I'll focus on the 4 and work through getting the complete model published/adopted as those have been in my branches for a while now. Still these patches get as far as the creation of the common object which is a start. The first 4 patches are essentially setup. Patch 5 is new and covers the need for nwfilters (at least temporarily until self locking list mgmt is possible). Patches 6-7 were more or less what patch 7-8 were in the RFC. Patches 8-9 are essentially the driver/common object extension. Patches 5-6 from the RFC still exist in my branches and would be the next logical step. It was a conscious decision to just use the "default" string comparison for hash table key. That key could be a UUID, but since the UUID can easily be converted from unsigned to signed - I believe it would be far simpler to keep that mechanism rather than complexity introduced by having "different" key search algorithms when the key isn't a char *. John Ferlan (9): util: Formatting cleanups to virobject API util: Introduce virObjectGetLockableObj util: Generate a common internal only error print util: Add magic_marker for all virObjects util: Introduce virObjectLockableRecursiveNew util: Introduce virObjectPoolableHashElement util: Add virObjectPoolableHashElementGet*Key util: Introduce virObjectPoolableDef util: Add virObjectPoolableDef* accessor API's src/libvirt_private.syms | 12 ++ src/util/virobject.c | 492 +++++++++++++++++++++++++++++++++++++++++++---- src/util/virobject.h | 130 +++++++++++-- 3 files changed, 580 insertions(+), 54 deletions(-) -- 2.9.4
Alter to use more recent formatting guidelines Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 64 ++++++++++++++++++++++++++++++++++------------------ src/util/virobject.h | 59 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 82 insertions(+), 41 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 51876b9..792685b 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -52,7 +52,8 @@ static virClassPtr virObjectLockableClass; static void virObjectLockableDispose(void *anyobj); -static int virObjectOnceInit(void) +static int +virObjectOnceInit(void) { if (!(virObjectClass = virClassNew(NULL, "virObject", @@ -77,7 +78,8 @@ VIR_ONCE_GLOBAL_INIT(virObject); * * Returns the class instance for the base virObject type */ -virClassPtr virClassForObject(void) +virClassPtr +virClassForObject(void) { if (virObjectInitialize() < 0) return NULL; @@ -91,7 +93,8 @@ virClassPtr virClassForObject(void) * * Returns the class instance for the virObjectLockable type */ -virClassPtr virClassForObjectLockable(void) +virClassPtr +virClassForObjectLockable(void) { if (virObjectInitialize() < 0) return NULL; @@ -116,10 +119,11 @@ virClassPtr virClassForObjectLockable(void) * * Returns a new class instance */ -virClassPtr virClassNew(virClassPtr parent, - const char *name, - size_t objectSize, - virObjectDisposeCallback dispose) +virClassPtr +virClassNew(virClassPtr parent, + const char *name, + size_t objectSize, + virObjectDisposeCallback dispose) { virClassPtr klass; @@ -162,8 +166,9 @@ virClassPtr virClassNew(virClassPtr parent, * * Return true if @klass is derived from @parent, false otherwise */ -bool virClassIsDerivedFrom(virClassPtr klass, - virClassPtr parent) +bool +virClassIsDerivedFrom(virClassPtr klass, + virClassPtr parent) { while (klass) { if (klass->magic == parent->magic) @@ -186,7 +191,8 @@ bool virClassIsDerivedFrom(virClassPtr klass, * * Returns the new object */ -void *virObjectNew(virClassPtr klass) +void * +virObjectNew(virClassPtr klass) { virObjectPtr obj = NULL; @@ -205,7 +211,8 @@ void *virObjectNew(virClassPtr klass) } -void *virObjectLockableNew(virClassPtr klass) +void * +virObjectLockableNew(virClassPtr klass) { virObjectLockablePtr obj; @@ -230,13 +237,15 @@ void *virObjectLockableNew(virClassPtr klass) } -static void virObjectLockableDispose(void *anyobj) +static void +virObjectLockableDispose(void *anyobj) { virObjectLockablePtr obj = anyobj; virMutexDestroy(&obj->lock); } + /** * virObjectUnref: * @anyobj: any instance of virObjectPtr @@ -248,7 +257,8 @@ static void virObjectLockableDispose(void *anyobj) * Returns true if the remaining reference count is * non-zero, false if the object was disposed of */ -bool virObjectUnref(void *anyobj) +bool +virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj; @@ -286,7 +296,8 @@ bool virObjectUnref(void *anyobj) * * Returns @anyobj */ -void *virObjectRef(void *anyobj) +void * +virObjectRef(void *anyobj) { virObjectPtr obj = anyobj; @@ -310,7 +321,8 @@ void *virObjectRef(void *anyobj) * The object must be unlocked before releasing this * reference. */ -void virObjectLock(void *anyobj) +void +virObjectLock(void *anyobj) { virObjectLockablePtr obj = anyobj; @@ -331,7 +343,8 @@ void virObjectLock(void *anyobj) * Release a lock on @anyobj. The lock must have been * acquired by virObjectLock. */ -void virObjectUnlock(void *anyobj) +void +virObjectUnlock(void *anyobj) { virObjectLockablePtr obj = anyobj; @@ -355,8 +368,9 @@ void virObjectUnlock(void *anyobj) * * Returns true if @anyobj is an instance of @klass */ -bool virObjectIsClass(void *anyobj, - virClassPtr klass) +bool +virObjectIsClass(void *anyobj, + virClassPtr klass) { virObjectPtr obj = anyobj; if (!obj) @@ -372,7 +386,8 @@ bool virObjectIsClass(void *anyobj, * * Returns the name of @klass */ -const char *virClassName(virClassPtr klass) +const char * +virClassName(virClassPtr klass) { return klass->name; } @@ -401,7 +416,9 @@ void virObjectFreeCallback(void *opaque) * but with the signature matching the virHashDataFree * typedef. */ -void virObjectFreeHashData(void *opaque, const void *name ATTRIBUTE_UNUSED) +void +virObjectFreeHashData(void *opaque, + const void *name ATTRIBUTE_UNUSED) { virObjectUnref(opaque); } @@ -413,7 +430,8 @@ void virObjectFreeHashData(void *opaque, const void *name ATTRIBUTE_UNUSED) * * Unrefs all members of @list and frees the list itself. */ -void virObjectListFree(void *list) +void +virObjectListFree(void *list) { void **next; @@ -434,7 +452,9 @@ void virObjectListFree(void *list) * * Unrefs all members of @list and frees the list itself. */ -void virObjectListFreeCount(void *list, size_t count) +void +virObjectListFreeCount(void *list, + size_t count) { size_t i; diff --git a/src/util/virobject.h b/src/util/virobject.h index c3ecc1e..f4c292b 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -66,40 +66,61 @@ virClassPtr virClassForObjectLockable(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) # endif -virClassPtr virClassNew(virClassPtr parent, - const char *name, - size_t objectSize, - virObjectDisposeCallback dispose) +virClassPtr +virClassNew(virClassPtr parent, + const char *name, + size_t objectSize, + virObjectDisposeCallback dispose) VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(2); -const char *virClassName(virClassPtr klass) +const char * +virClassName(virClassPtr klass) ATTRIBUTE_NONNULL(1); -bool virClassIsDerivedFrom(virClassPtr klass, - virClassPtr parent) +bool +virClassIsDerivedFrom(virClassPtr klass, + virClassPtr parent) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -void *virObjectNew(virClassPtr klass) +void * +virObjectNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); -bool virObjectUnref(void *obj); -void *virObjectRef(void *obj); -bool virObjectIsClass(void *obj, - virClassPtr klass) +bool +virObjectUnref(void *obj); + +void * +virObjectRef(void *obj); + +bool +virObjectIsClass(void *obj, + virClassPtr klass) ATTRIBUTE_NONNULL(2); -void virObjectFreeCallback(void *opaque); -void virObjectFreeHashData(void *opaque, const void *name); +void +virObjectFreeCallback(void *opaque); + +void +virObjectFreeHashData(void *opaque, + const void *name); -void *virObjectLockableNew(virClassPtr klass) +void * +virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); -void virObjectLock(void *lockableobj) +void +virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -void virObjectUnlock(void *lockableobj) + +void +virObjectUnlock(void *lockableobj) ATTRIBUTE_NONNULL(1); -void virObjectListFree(void *list); -void virObjectListFreeCount(void *list, size_t count); +void +virObjectListFree(void *list); + +void +virObjectListFreeCount(void *list, + size_t count); #endif /* __VIR_OBJECT_H */ -- 2.9.4
Split out the object fetch in virObject{Lock|Unlock} into a helper Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 792685b..34805d3 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -309,6 +309,22 @@ 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 virObjectLockablePtr @@ -324,13 +340,10 @@ virObjectRef(void *anyobj) void virObjectLock(void *anyobj) { - virObjectLockablePtr obj = anyobj; + virObjectLockablePtr obj = virObjectGetLockableObj(anyobj); - if (!virObjectIsClass(obj, virObjectLockableClass)) { - VIR_WARN("Object %p (%s) is not a virObjectLockable instance", - obj, obj ? obj->parent.klass->name : "(unknown)"); + if (!obj) return; - } virMutexLock(&obj->lock); } @@ -346,13 +359,10 @@ virObjectLock(void *anyobj) void virObjectUnlock(void *anyobj) { - virObjectLockablePtr obj = anyobj; + virObjectLockablePtr obj = virObjectGetLockableObj(anyobj); - if (!virObjectIsClass(obj, virObjectLockableClass)) { - VIR_WARN("Object %p (%s) is not a virObjectLockable instance", - obj, obj ? obj->parent.klass->name : "(unknown)"); + if (!obj) return; - } virMutexUnlock(&obj->lock); } -- 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 | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 34805d3..9f5f187 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,6 +47,16 @@ struct _virClass { virObjectDisposeCallback dispose; }; +#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ + do { \ + virObjectPtr obj = anyobj; \ + if (!obj) \ + VIR_WARN("Object %p is not a virObject class instance", anyobj);\ + else \ + VIR_WARN("Object %p (%s) is not a %s instance", \ + anyobj, obj->klass->name, #objclass); \ + } while (0) + static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; @@ -312,14 +322,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, virObjectLockableClass); return NULL; } -- 2.9.4
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, let's add a "magic_marker" to the base virObject class that contains a value "0xFEEDBEEF", then any place in the code which would accept or process the base opaque virObjectPtr, compare the magic_marker against 0xFEEDBEEF to make sure this is an object allocated by this code. 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 | 12 ++++++++---- src/util/virobject.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 9f5f187..a1934941 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,10 +47,12 @@ struct _virClass { virObjectDisposeCallback dispose; }; +#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ + if (VIR_OBJECT_NOTVALID(obj)) \ VIR_WARN("Object %p is not a virObject class instance", anyobj);\ else \ VIR_WARN("Object %p (%s) is not a %s instance", \ @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass) return NULL; obj->u.s.magic = klass->magic; + obj->magic_marker = 0xFEEDBEEF; obj->klass = klass; virAtomicIntSet(&obj->u.s.refs, 1); @@ -272,7 +275,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false; bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj) /* Clear & poison object */ memset(obj, 0, obj->klass->objectSize); obj->u.s.magic = 0xDEADBEEF; + obj->magic_marker = 0xDEADBEEF; obj->klass = (void*)0xDEADBEEF; VIR_FREE(obj); } @@ -311,7 +315,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); @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false; return virClassIsDerivedFrom(obj->klass, klass); diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b..89f8050 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -51,6 +51,7 @@ struct _virObject { int refs; } s; } u; + unsigned int magic_marker; virClassPtr klass; }; -- 2.9.4
On Tue, May 30, 2017 at 07:38:16AM -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, let's add a "magic_marker" to the base virObject class that contains a value "0xFEEDBEEF", then any place in the code which would accept or process the base opaque virObjectPtr, compare the magic_marker against 0xFEEDBEEF to make sure this is an object allocated by this code.
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.
I don't think that we need to guard this kind of programming error. We've been able to cope with the current virObject code without hitting this issue. Pavel
On 05/30/2017 08:50 AM, Pavel Hrdina wrote:
On Tue, May 30, 2017 at 07:38:16AM -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, let's add a "magic_marker" to the base virObject class that contains a value "0xFEEDBEEF", then any place in the code which would accept or process the base opaque virObjectPtr, compare the magic_marker against 0xFEEDBEEF to make sure this is an object allocated by this code.
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.
I don't think that we need to guard this kind of programming error. We've been able to cope with the current virObject code without hitting this issue.
I don't disagree that we've been able to cope, but adding more objects makes things more interesting and prone to having code trip across a bad usage which we could have prevented. The base object classes make a lot of assumptions and use opaque params, so having some sort of check prevents those possibly very bad things happening. Not everyone is well behaved and at times we do make assumptions. IIRC it took me a bit of combing through what I'd changed to figure out my erroneous usage which resulted in me adding this to make it very much so more obvious. John
On Tue, May 30, 2017 at 07:38:16AM -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, let's add a "magic_marker" to the base virObject class that contains a value "0xFEEDBEEF", then any place in the code which would accept or process the base opaque virObjectPtr, compare the magic_marker against 0xFEEDBEEF to make sure this is an object allocated by this code.
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 | 12 ++++++++---- src/util/virobject.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 9f5f187..a1934941 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,10 +47,12 @@ struct _virClass { virObjectDisposeCallback dispose; };
+#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ + if (VIR_OBJECT_NOTVALID(obj)) \ VIR_WARN("Object %p is not a virObject class instance", anyobj);\ else \ VIR_WARN("Object %p (%s) is not a %s instance", \ @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass) return NULL;
obj->u.s.magic = klass->magic; + obj->magic_marker = 0xFEEDBEEF; obj->klass = klass; virAtomicIntSet(&obj->u.s.refs, 1);
@@ -272,7 +275,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj;
- if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj) /* Clear & poison object */ memset(obj, 0, obj->klass->objectSize); obj->u.s.magic = 0xDEADBEEF; + obj->magic_marker = 0xDEADBEEF; obj->klass = (void*)0xDEADBEEF; VIR_FREE(obj); } @@ -311,7 +315,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); @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
return virClassIsDerivedFrom(obj->klass, klass); diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b..89f8050 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -51,6 +51,7 @@ struct _virObject { int refs; } s; } u; + unsigned int magic_marker; virClassPtr klass; };
I'm wondering whether this will risk re-introducing the bug fixed in commit fca4f2334072d87f7faeb2948e6f83201309e1b9 Author: Eric Blake <eblake@redhat.com> Date: Thu Dec 12 16:01:15 2013 -0700 object: require maximal alignment in base class I'm also thinking we don't really need to have 2 magic fields in the same struct - we already have a 'magic' field that is initialized from the class object magic value. Now, this existing magic is different for each object subclass - we allocate class magic starting with static unsigned int magicCounter = 0xCAFE0000; I'm thinking though, that we're never going to have > 65556 different sub-classes (well at least not for a long time). So instead of adding this new field you could just check ((object->u.s.magic & 0xCAFE0000) == 0xCAFE0000) 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 :|
On 05/30/2017 09:00 AM, Daniel P. Berrange wrote:
On Tue, May 30, 2017 at 07:38:16AM -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, let's add a "magic_marker" to the base virObject class that contains a value "0xFEEDBEEF", then any place in the code which would accept or process the base opaque virObjectPtr, compare the magic_marker against 0xFEEDBEEF to make sure this is an object allocated by this code.
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 | 12 ++++++++---- src/util/virobject.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 9f5f187..a1934941 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,10 +47,12 @@ struct _virClass { virObjectDisposeCallback dispose; };
+#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ + if (VIR_OBJECT_NOTVALID(obj)) \ VIR_WARN("Object %p is not a virObject class instance", anyobj);\ else \ VIR_WARN("Object %p (%s) is not a %s instance", \ @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass) return NULL;
obj->u.s.magic = klass->magic; + obj->magic_marker = 0xFEEDBEEF; obj->klass = klass; virAtomicIntSet(&obj->u.s.refs, 1);
@@ -272,7 +275,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj;
- if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj) /* Clear & poison object */ memset(obj, 0, obj->klass->objectSize); obj->u.s.magic = 0xDEADBEEF; + obj->magic_marker = 0xDEADBEEF; obj->klass = (void*)0xDEADBEEF; VIR_FREE(obj); } @@ -311,7 +315,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); @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
return virClassIsDerivedFrom(obj->klass, klass); diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b..89f8050 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -51,6 +51,7 @@ struct _virObject { int refs; } s; } u; + unsigned int magic_marker; virClassPtr klass; };
I'm wondering whether this will risk re-introducing the bug fixed in
commit fca4f2334072d87f7faeb2948e6f83201309e1b9 Author: Eric Blake <eblake@redhat.com> Date: Thu Dec 12 16:01:15 2013 -0700
object: require maximal alignment in base class
I'm also thinking we don't really need to have 2 magic fields in the same struct - we already have a 'magic' field that is initialized from the class object magic value.
Now, this existing magic is different for each object subclass - we allocate class magic starting with
static unsigned int magicCounter = 0xCAFE0000;
I'm thinking though, that we're never going to have > 65556 different sub-classes (well at least not for a long time).
So instead of adding this new field you could just check
((object->u.s.magic & 0xCAFE0000) == 0xCAFE0000)
Oh right - just mask away the pesky counter portion... This works me too. I can adjust. Tks - John
On Tue, May 30, 2017 at 10:43:35AM -0400, John Ferlan wrote:
On 05/30/2017 09:00 AM, Daniel P. Berrange wrote:
On Tue, May 30, 2017 at 07:38:16AM -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, let's add a "magic_marker" to the base virObject class that contains a value "0xFEEDBEEF", then any place in the code which would accept or process the base opaque virObjectPtr, compare the magic_marker against 0xFEEDBEEF to make sure this is an object allocated by this code.
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 | 12 ++++++++---- src/util/virobject.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 9f5f187..a1934941 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -47,10 +47,12 @@ struct _virClass { virObjectDisposeCallback dispose; };
+#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF) + #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \ do { \ virObjectPtr obj = anyobj; \ - if (!obj) \ + if (VIR_OBJECT_NOTVALID(obj)) \ VIR_WARN("Object %p is not a virObject class instance", anyobj);\ else \ VIR_WARN("Object %p (%s) is not a %s instance", \ @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass) return NULL;
obj->u.s.magic = klass->magic; + obj->magic_marker = 0xFEEDBEEF; obj->klass = klass; virAtomicIntSet(&obj->u.s.refs, 1);
@@ -272,7 +275,7 @@ virObjectUnref(void *anyobj) { virObjectPtr obj = anyobj;
- if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs); @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj) /* Clear & poison object */ memset(obj, 0, obj->klass->objectSize); obj->u.s.magic = 0xDEADBEEF; + obj->magic_marker = 0xDEADBEEF; obj->klass = (void*)0xDEADBEEF; VIR_FREE(obj); } @@ -311,7 +315,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); @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj, virClassPtr klass) { virObjectPtr obj = anyobj; - if (!obj) + if (VIR_OBJECT_NOTVALID(obj)) return false;
return virClassIsDerivedFrom(obj->klass, klass); diff --git a/src/util/virobject.h b/src/util/virobject.h index f4c292b..89f8050 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -51,6 +51,7 @@ struct _virObject { int refs; } s; } u; + unsigned int magic_marker; virClassPtr klass; };
I'm wondering whether this will risk re-introducing the bug fixed in
commit fca4f2334072d87f7faeb2948e6f83201309e1b9 Author: Eric Blake <eblake@redhat.com> Date: Thu Dec 12 16:01:15 2013 -0700
object: require maximal alignment in base class
I'm also thinking we don't really need to have 2 magic fields in the same struct - we already have a 'magic' field that is initialized from the class object magic value.
Now, this existing magic is different for each object subclass - we allocate class magic starting with
static unsigned int magicCounter = 0xCAFE0000;
I'm thinking though, that we're never going to have > 65556 different sub-classes (well at least not for a long time).
So instead of adding this new field you could just check
((object->u.s.magic & 0xCAFE0000) == 0xCAFE0000)
Oh right - just mask away the pesky counter portion... This works me too. I can adjust.
Should also put an assert in the virClassNew method that the new magic is <= 0xCAFEFFFF as a safety net for 20 years time when we finally have 65536 classes :-) 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 recursive option for the lockable object which calls virMutexInitRecursive instead of virMutexInit. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 26 ++++++++++++++++++++++++++ src/util/virobject.h | 4 ++++ 3 files changed, 31 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 429b095..9693dc4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2268,6 +2268,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLockableRecursiveNew; virObjectNew; virObjectRef; virObjectUnlock; diff --git a/src/util/virobject.c b/src/util/virobject.c index a1934941..b1fabd7 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -250,6 +250,32 @@ virObjectLockableNew(virClassPtr klass) } +void * +virObjectLockableRecursiveNew(virClassPtr klass) +{ + virObjectLockablePtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectLockable())) { + virReportInvalidArg(klass, + _("Class %s must derive from virObjectLockable"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectNew(klass))) + return NULL; + + if (virMutexInitRecursive(&obj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize recursive mutex")); + virObjectUnref(obj); + return NULL; + } + + return obj; +} + + static void virObjectLockableDispose(void *anyobj) { diff --git a/src/util/virobject.h b/src/util/virobject.h index 89f8050..7df9b47 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -109,6 +109,10 @@ void * virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectLockableRecursiveNew(virClassPtr klass) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
Introduce a recursive option for the lockable object which calls virMutexInitRecursive instead of virMutexInit.
We should avoid using recursive lock because they are dangerous. I would rather cleanup the nwfilter code than introducing more stuff for recursive locks. Pavel
On Tue, May 30, 2017 at 03:03:14PM +0200, Pavel Hrdina wrote:
On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
Introduce a recursive option for the lockable object which calls virMutexInitRecursive instead of virMutexInit.
We should avoid using recursive lock because they are dangerous. I would rather cleanup the nwfilter code than introducing more stuff for recursive locks.
Last time I looked I came to the conclusion that it wasn't possible to remove the recursive locking from nwfilter. I agree about /not/ introducing new usage of recursive locking though. 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 :|
On 05/30/2017 09:19 AM, Daniel P. Berrange wrote:
On Tue, May 30, 2017 at 03:03:14PM +0200, Pavel Hrdina wrote:
On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
Introduce a recursive option for the lockable object which calls virMutexInitRecursive instead of virMutexInit.
We should avoid using recursive lock because they are dangerous. I would rather cleanup the nwfilter code than introducing more stuff for recursive locks.
Last time I looked I came to the conclusion that it wasn't possible to remove the recursive locking from nwfilter.
I agree about /not/ introducing new usage of recursive locking though.
So this is a chicken/egg problem.... Cannot create a new API to use recursive locks and cannot remove the recursive lock without having adjustments to nwfilter code that would seemingly require self locking list objects to get hash table elements. If the comments in src/nwfilter/nwfilter_gentech_driver.c (search on XXX) could prove to be true, then it would be possible to not have a recursive lock. Perhaps that loop could be rewritten in virnwfilterobj to grab a reference to a hash table object while releasing the object lock that's causing the deadlock. I haven't yet circled back to determine whether this is possible yet, but since code in my branches is at least this far, I can look again. John
Add a new virObjectLockable child which will be used to more generically describe driver objects. Eventually these objects will be placed into a more generic hash table object which will take care of object mgmt functions. Each virObjectPoolableHashElement will have a primaryKey (required) and a secondaryKey (optional) which can be used to insert the same object into two hash tables for faster lookups. The recursive parameter controls which type of lock is used. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 18 +++++++++++ 3 files changed, 101 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9693dc4..eca1980 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2258,6 +2258,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectPoolableHashElement; virClassIsDerivedFrom; virClassName; virClassNew; @@ -2270,6 +2271,7 @@ virObjectLock; virObjectLockableNew; virObjectLockableRecursiveNew; virObjectNew; +virObjectPoolableHashElementNew; virObjectRef; virObjectUnlock; virObjectUnref; diff --git a/src/util/virobject.c b/src/util/virobject.c index b1fabd7..25dbb8f 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -61,8 +61,10 @@ struct _virClass { static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; +static virClassPtr virObjectPoolableHashElementClass; static void virObjectLockableDispose(void *anyobj); +static void virObjectPoolableHashElementDispose(void *anyobj); static int virObjectOnceInit(void) @@ -79,6 +81,13 @@ virObjectOnceInit(void) virObjectLockableDispose))) return -1; + if (!(virObjectPoolableHashElementClass = + virClassNew(virObjectLockableClass, + "virObjectPoolableHashElement", + sizeof(virObjectPoolableHashElement), + virObjectPoolableHashElementDispose))) + return -1; + return 0; } @@ -116,6 +125,23 @@ virClassForObjectLockable(void) /** + * virClassForObjectPoolableHashElement: + * + * Returns the class instance for the virObjectPoolableHashElement type + */ +virClassPtr +virClassForObjectPoolableHashElement(void) +{ + if (virObjectInitialize() < 0) + return NULL; + + VIR_DEBUG("virObjectPoolableHashElementClass=%p", + virObjectPoolableHashElementClass); + return virObjectPoolableHashElementClass; +} + + +/** * virClassNew: * @parent: the parent class * @name: the class name @@ -285,6 +311,59 @@ virObjectLockableDispose(void *anyobj) } +void * +virObjectPoolableHashElementNew(virClassPtr klass, + bool recursive, + const char *primaryKey, + const char *secondaryKey) +{ + virObjectPoolableHashElementPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectPoolableHashElement())) { + virReportInvalidArg(klass, + _("Class %s must derive from " + "virObjectPoolableHashElement"), + virClassName(klass)); + return NULL; + } + + if (!recursive) { + if (!(obj = virObjectLockableNew(klass))) + return NULL; + } else { + if (!(obj = virObjectLockableRecursiveNew(klass))) + return NULL; + } + + if (VIR_STRDUP(obj->primaryKey, primaryKey) < 0) + goto error; + + if (secondaryKey && VIR_STRDUP(obj->secondaryKey, secondaryKey) < 0) + goto error; + + VIR_DEBUG("obj=%p, primary=%s secondary=%s", + obj, obj->primaryKey, NULLSTR(obj->secondaryKey)); + + return obj; + + error: + virObjectUnref(obj); + return NULL; +} + + +static void +virObjectPoolableHashElementDispose(void *anyobj) +{ + virObjectPoolableHashElementPtr obj = anyobj; + + VIR_DEBUG("dispose obj=%p", obj); + + VIR_FREE(obj->primaryKey); + VIR_FREE(obj->secondaryKey); +} + + /** * virObjectUnref: * @anyobj: any instance of virObjectPtr @@ -352,7 +431,8 @@ virObjectRef(void *anyobj) static virObjectLockablePtr virObjectGetLockableObj(void *anyobj) { - if (virObjectIsClass(anyobj, virObjectLockableClass)) + if (virObjectIsClass(anyobj, virObjectLockableClass) || + virObjectIsClass(anyobj, virObjectPoolableHashElementClass)) return anyobj; VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockableClass); diff --git a/src/util/virobject.h b/src/util/virobject.h index 7df9b47..0377cd5 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -34,6 +34,9 @@ typedef virObject *virObjectPtr; typedef struct _virObjectLockable virObjectLockable; typedef virObjectLockable *virObjectLockablePtr; +typedef struct _virObjectPoolableHashElement virObjectPoolableHashElement; +typedef virObjectPoolableHashElement *virObjectPoolableHashElementPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -60,9 +63,17 @@ struct _virObjectLockable { virMutex lock; }; +struct _virObjectPoolableHashElement { + virObjectLockable parent; + + char *primaryKey; + char *secondaryKey; +}; + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); +virClassPtr virClassForObjectPoolableHashElement(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -113,6 +124,13 @@ void * virObjectLockableRecursiveNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectPoolableHashElementNew(virClassPtr klass, + bool recursive, + const char *primaryKey, + const char *secondaryKey) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Add a pair of accessor API's: virObjectPoolableHashElementGetPrimaryKey virObjectPoolableHashElementGetSecondaryKey which will return the requested key from the object to the caller. It is up to the caller to check the returned key and error if the return value is NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 6 ++++++ 3 files changed, 58 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eca1980..fea0be7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2271,6 +2271,8 @@ virObjectLock; virObjectLockableNew; virObjectLockableRecursiveNew; virObjectNew; +virObjectPoolableHashElementGetPrimaryKey; +virObjectPoolableHashElementGetSecondaryKey; virObjectPoolableHashElementNew; virObjectRef; virObjectUnlock; diff --git a/src/util/virobject.c b/src/util/virobject.c index 25dbb8f..74299f8 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -441,6 +441,18 @@ virObjectGetLockableObj(void *anyobj) } +static virObjectPoolableHashElementPtr +virObjectGetPoolableHashElementObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectPoolableHashElementClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectPoolableHashElementClass); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockablePtr @@ -592,3 +604,41 @@ virObjectListFreeCount(void *list, VIR_FREE(list); } + + +/** + * virObjectPoolableHashElementGetPrimaryKey + * @anyobj: Pointer to a PoolableHashElement object + * + * Returns: Pointer to the primaryKey or NULL on failure + */ +const char * +virObjectPoolableHashElementGetPrimaryKey(void *anyobj) +{ + virObjectPoolableHashElementPtr obj = + virObjectGetPoolableHashElementObj(anyobj); + + if (!obj) + return NULL; + + return obj->primaryKey; +} + + +/** + * virObjectPoolableHashElementGetSecondaryKey + * @anyobj: Pointer to a PoolableHashElement object + * + * Returns: Pointer to the secondaryKey which may be NULL + */ +const char * +virObjectPoolableHashElementGetSecondaryKey(void *anyobj) +{ + virObjectPoolableHashElementPtr obj = + virObjectGetPoolableHashElementObj(anyobj); + + if (!obj) + return NULL; + + return obj->secondaryKey; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 0377cd5..4706502 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -146,4 +146,10 @@ void virObjectListFreeCount(void *list, size_t count); +const char * +virObjectPoolableHashElementGetPrimaryKey(void *anyobj); + +const char * +virObjectPoolableHashElementGetSecondaryKey(void *anyobj); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
Add a new virObjectPoolableHashElement child which will be used to provide the basis for driver def/newDef elements. Each virObjectPoolableDef has: 1. A @recursive argument to denote which type of locks to use. 2. A required @primaryKey to be used to uniquely identity the object by some string value. 3. An optional @secondaryKey to be used as a secondary means of search for the object by some string value. 4. A required @def and @defFreeFunc. The @def will be consumed by the object and when disposed the free function will be called. The _virObjectPoolableDef has an additional @newDef element to store the "next" boot configuration for consumers that support the functionality. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 25 ++++++++++++++ 3 files changed, 111 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fea0be7..4fad7c8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2258,6 +2258,7 @@ virNumaSetupMemoryPolicy; # util/virobject.h virClassForObject; virClassForObjectLockable; +virClassForObjectPoolableDef; virClassForObjectPoolableHashElement; virClassIsDerivedFrom; virClassName; @@ -2271,6 +2272,7 @@ virObjectLock; virObjectLockableNew; virObjectLockableRecursiveNew; virObjectNew; +virObjectPoolableDefNew; virObjectPoolableHashElementGetPrimaryKey; virObjectPoolableHashElementGetSecondaryKey; virObjectPoolableHashElementNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 74299f8..d1fc3f0 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -62,9 +62,11 @@ struct _virClass { static virClassPtr virObjectClass; static virClassPtr virObjectLockableClass; static virClassPtr virObjectPoolableHashElementClass; +static virClassPtr virObjectPoolableDefClass; static void virObjectLockableDispose(void *anyobj); static void virObjectPoolableHashElementDispose(void *anyobj); +static void virObjectPoolableDefDispose(void *anyobj); static int virObjectOnceInit(void) @@ -88,6 +90,13 @@ virObjectOnceInit(void) virObjectPoolableHashElementDispose))) return -1; + if (!(virObjectPoolableDefClass = + virClassNew(virObjectPoolableHashElementClass, + "virObjectPoolableDef", + sizeof(virObjectPoolableDef), + virObjectPoolableDefDispose))) + return -1; + return 0; } @@ -142,6 +151,23 @@ virClassForObjectPoolableHashElement(void) /** + * virClassForObjectPoolableDef: + * + * Returns the class instance for the virObjectPoolableDef type + */ +virClassPtr +virClassForObjectPoolableDef(void) +{ + if (virObjectInitialize() < 0) + return NULL; + + VIR_DEBUG("virObjectPoolableDefClass=%p", + virObjectPoolableDefClass); + return virObjectPoolableDefClass; +} + + +/** * virClassNew: * @parent: the parent class * @name: the class name @@ -365,6 +391,62 @@ virObjectPoolableHashElementDispose(void *anyobj) /** + * virObjectPoolableDefNew: + * @klass: the klass to check + * @recursive: boolean to dictate which Lockable object to use + * @primaryKey: primary key (required) + * @secondaryKey: secondary key + * @def: XML definition (required) + * @defFreeFunc: Free function for @def and @newDef (required) + * + * Create a new poolable def object for storing "common" domain defs. + * + * Returns: New object on success, NULL on failure w/ error message set + */ +void * +virObjectPoolableDefNew(virClassPtr klass, + bool recursive, + const char *primaryKey, + const char *secondaryKey, + void *def, + virFreeCallback defFreeFunc) +{ + virObjectPoolableDefPtr obj; + + if (!virClassIsDerivedFrom(klass, virClassForObjectPoolableDef())) { + virReportInvalidArg(klass, + _("Class %s must derive from " + "virObjectPoolableDef"), + virClassName(klass)); + return NULL; + } + + if (!(obj = virObjectPoolableHashElementNew(klass, recursive, + primaryKey, secondaryKey))) + return NULL; + + obj->def = def; + obj->defFreeFunc = defFreeFunc; + + VIR_DEBUG("obj=%p, def=%p ff=%p", obj, obj->def, obj->defFreeFunc); + + return obj; +} + + +static void +virObjectPoolableDefDispose(void *anyobj) +{ + virObjectPoolableDefPtr obj = anyobj; + + VIR_DEBUG("dispose obj=%p", obj); + + (obj->defFreeFunc)(obj->def); + (obj->defFreeFunc)(obj->newDef); +} + + +/** * virObjectUnref: * @anyobj: any instance of virObjectPtr * @@ -432,7 +514,8 @@ static virObjectLockablePtr virObjectGetLockableObj(void *anyobj) { if (virObjectIsClass(anyobj, virObjectLockableClass) || - virObjectIsClass(anyobj, virObjectPoolableHashElementClass)) + virObjectIsClass(anyobj, virObjectPoolableHashElementClass) || + virObjectIsClass(anyobj, virObjectPoolableDefClass)) return anyobj; VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockableClass); diff --git a/src/util/virobject.h b/src/util/virobject.h index 4706502..9bbbece 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -37,6 +37,9 @@ typedef virObjectLockable *virObjectLockablePtr; typedef struct _virObjectPoolableHashElement virObjectPoolableHashElement; typedef virObjectPoolableHashElement *virObjectPoolableHashElementPtr; +typedef struct _virObjectPoolableDef virObjectPoolableDef; +typedef virObjectPoolableDef *virObjectPoolableDefPtr; + typedef void (*virObjectDisposeCallback)(void *obj); /* Most code should not play with the contents of this struct; however, @@ -70,10 +73,22 @@ struct _virObjectPoolableHashElement { char *secondaryKey; }; +struct _virObjectPoolableDef { + virObjectPoolableHashElement parent; + + /* 'def' is the current config definition. + * 'newDef' is the next boot configuration. + */ + void *def; + void *newDef; + virFreeCallback defFreeFunc; +}; + virClassPtr virClassForObject(void); virClassPtr virClassForObjectLockable(void); virClassPtr virClassForObjectPoolableHashElement(void); +virClassPtr virClassForObjectPoolableDef(void); # ifndef VIR_PARENT_REQUIRED # define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1) @@ -131,6 +146,16 @@ virObjectPoolableHashElementNew(virClassPtr klass, const char *secondaryKey) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); +void * +virObjectPoolableDefNew(virClassPtr klass, + bool recursive, + const char *primaryKey, + const char *secondaryKey, + void *def, + virFreeCallback defFreeFunc) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5) + ATTRIBUTE_NONNULL(6); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4
Add new API's for object management: virObjectPoolableDefGetDef - Just return the obj->def from the object. virObjectPoolableDefSetDef - If the new @def is non-NULL, replace the previous obj->def with the new @def argument calling the obj->defFreeFunc on the previous obj->def. If the @def is NULL, then just clear the obj->def leaving the caller to handle removal of the previous obj->def. virObjectPoolableDefGetNewDef - Just return the obj->newDef from the object. virObjectPoolableDefSetNewDef - If the new @newDef is non-NULL, replace the previous obj->newDef with the new @newDef argument calling the obj->defFreeFunc on the previous obj->newDef. If the @newDef is NULL, then just clear the obj->newDef leaving the caller to handle removal of the previous obj->newDef. virObjectPoolableDefSwapNewDef - Manage swapping the obj->newDef into obj->def by first calling the obj->defFreeFunc on the current obj->def and then stealing the obj->newDef into obj->def. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 5 ++ src/util/virobject.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 17 ++++++ 3 files changed, 165 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4fad7c8..f6e40c7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2272,7 +2272,12 @@ virObjectLock; virObjectLockableNew; virObjectLockableRecursiveNew; virObjectNew; +virObjectPoolableDefGetDef; +virObjectPoolableDefGetNewDef; virObjectPoolableDefNew; +virObjectPoolableDefSetDef; +virObjectPoolableDefSetNewDef; +virObjectPoolableDefStealNewDef; virObjectPoolableHashElementGetPrimaryKey; virObjectPoolableHashElementGetSecondaryKey; virObjectPoolableHashElementNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index d1fc3f0..ef253b6 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -536,6 +536,18 @@ virObjectGetPoolableHashElementObj(void *anyobj) } +static virObjectPoolableDefPtr +virObjectGetPoolableDefObj(void *anyobj) +{ + if (virObjectIsClass(anyobj, virObjectPoolableDefClass)) + return anyobj; + + VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectPoolableDefClass); + + return NULL; +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockablePtr @@ -725,3 +737,134 @@ virObjectPoolableHashElementGetSecondaryKey(void *anyobj) return obj->secondaryKey; } + + +/** + * virObjectPoolableDefGetDef + * @anyobj: Pointer to a locked PoolableDef object + * + * Returns: Pointer to the object @def or NULL on failure + */ +void * +virObjectPoolableDefGetDef(void *anyobj) +{ + virObjectPoolableDefPtr obj = virObjectGetPoolableDefObj(anyobj); + + if (!obj) + return NULL; + + return obj->def; +} + + +/** + * virObjectPoolableDefSetDef + * @anyobj: Pointer to a locked PoolableDef object + * @def: Opaque object definition to replace in the object + * + * Set the @def of the object - this may be NULL if clearing the @def + * from the object in which case it is up to the caller to handle managing + * the previous obj->def. If @def is not NULL, this includes a call to the + * defFreeFunc prior to the set. + * + * Returns 0 on success, -1 on failure + */ +int +virObjectPoolableDefSetDef(void *anyobj, + void *def) +{ + virObjectPoolableDefPtr obj = virObjectGetPoolableDefObj(anyobj); + + if (!obj) + return -1; + + if (def) { + obj->defFreeFunc(obj->def); + obj->def = def; + } else { + obj->def = NULL; + } + + return 0; +} + + +/** + * virObjectPoolableDefGetNewDef + * @anyobj: Pointer to a locked PoolableDef object + * + * Returns: Pointer to the object 'newDef' or NULL on failure + */ +void * +virObjectPoolableDefGetNewDef(void *anyobj) +{ + virObjectPoolableDefPtr obj = virObjectGetPoolableDefObj(anyobj); + + if (!obj) + return NULL; + + return obj->newDef; +} + + +/** + * virObjectPoolableDefSetNewDef + * @anyobj: Pointer to a locked PoolableDef object + * @newDef: Opaque 'newDef' to replace in the object, may be NULL + * + * Set the 'newDef' of the object - this may be NULL if clearing the newDef + * from the object in which case it is up to the caller to handle managing + * the previous newDef. If @newDef is not NULL, this includes a call to the + * defFreeFunc prior to the set. + * + * Returns 0 on success, -1 on failure + */ +int +virObjectPoolableDefSetNewDef(void *anyobj, + void *newDef) +{ + virObjectPoolableDefPtr obj = virObjectGetPoolableDefObj(anyobj); + + if (!obj) + return -1; + + if (newDef) { + obj->defFreeFunc(obj->newDef); + obj->newDef = newDef; + } else { + obj->newDef = NULL; + } + + return 0; +} + + +/** + * virObjectPoolableDefSwapNewDef + * @anyobj: Pointer to a locked PoolableDef object + * + * Manage swapping the obj->newDef into obj->def in one pass. + * When called, the previous obj->def will be free'd and the + * existing defDef will take it's place. + * + * Returns 0 on success, -1 on failure + */ +int +virObjectPoolableDefStealNewDef(void *anyobj) +{ + virObjectPoolableDefPtr obj = virObjectGetPoolableDefObj(anyobj); + + if (!obj) + return -1; + + if (!obj->newDef) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to steal NULL newDef")); + return -1; + } + + obj->defFreeFunc(obj->def); + VIR_STEAL_PTR(obj->def, obj->newDef); + + return 0; +} diff --git a/src/util/virobject.h b/src/util/virobject.h index 9bbbece..d3aeb73 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -177,4 +177,21 @@ virObjectPoolableHashElementGetPrimaryKey(void *anyobj); const char * virObjectPoolableHashElementGetSecondaryKey(void *anyobj); +void * +virObjectPoolableDefGetDef(void *anyobj); + +int +virObjectPoolableDefSetDef(void *anyobj, + void *def); + +void * +virObjectPoolableDefGetNewDef(void *anyobj); + +int +virObjectPoolableDefSetNewDef(void *anyobj, + void *newDef); + +int +virObjectPoolableDefStealNewDef(void *anyobj); + #endif /* __VIR_OBJECT_H */ -- 2.9.4
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Pavel Hrdina