Alter to use more recent formatting guidelines Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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(a)redhat.com&gt; --- 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

On 06/05/2017 03:39 AM, Peter Krempa wrote: True; however, if you had snipped more context: - VIR_WARN("Object %p (%s) is not a virObjectLockable instance", - anyobj, obj ? obj->klass->name : "(unknown)"); the old context used the "? ... :" and would also have said "Object (nil) (unknown) is not... " - I just went more direct and removed the assumption that it's a virObjectLockable instance. I can change the message if that's all you object to though. The goal here was the second half of the message to be more explicit. John

On Fri, Jun 02, 2017 at 06:17:18 -0400, John Ferlan wrote: [..] Library code should not use assert. This makes the client application crash on library usage. This also does not deterministically make this crash all the time. Only if you initialize the class that exceeds the marker, which depends on serialization of the initialization.

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. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 17 +++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 429b095..a400d8f 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; @@ -2269,6 +2270,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectPoolableHashElementNew; virObjectRef; virObjectUnlock; virObjectUnref; diff --git a/src/util/virobject.c b/src/util/virobject.c index e0465b1..302b7ac 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -62,8 +62,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) @@ -80,6 +82,13 @@ virObjectOnceInit(void) virObjectLockableDispose))) return -1; + if (!(virObjectPoolableHashElementClass = + virClassNew(virObjectLockableClass, + "virObjectPoolableHashElement", + sizeof(virObjectPoolableHashElement), + virObjectPoolableHashElementDispose))) + return -1; + return 0; } @@ -117,6 +126,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 @@ -260,6 +286,53 @@ virObjectLockableDispose(void *anyobj) } +void * +virObjectPoolableHashElementNew(virClassPtr klass, + 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 (!(obj = virObjectLockableNew(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 @@ -326,7 +399,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 f4c292b..e29dae7 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, @@ -59,9 +62,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) @@ -108,6 +119,12 @@ void * virObjectLockableNew(virClassPtr klass) ATTRIBUTE_NONNULL(1); +void * +virObjectPoolableHashElementNew(virClassPtr klass, + const char *primaryKey, + const char *secondaryKey) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4

On 06/05/2017 08:25 AM, Peter Krempa wrote: They're not in a single hash table. Honestly, your comment makes no sense to me in light of how _virDomainObj[List] manages to assign a single object into two hash tables. What these objects do is take that abstraction "back" a level into the object. It's what was I believe suggested from comments of the RFC I had posted in Jan, but got reviewed in Feb - the link to the review is: https://www.redhat.com/archives/libvir-list/2017-February/msg00521.html Huh? Isn't an object by it's nature supposed to be opaque? Do you really care that virObjectLockable can be both lockable via virObjectLock and virObjectUnlock (e.g. virMutex and virMutexLock and virMutexUnlock) as well as being used to increase or decrease a Ref count via virObjectRef and virObjectUnref (e.g. virAtomic* APIs). No one cares any more about the guts because they trust the APIs. The guts of how that Lock/Unlock are done and how that Ref/Unref are done is hidden by the object logic. Similarly, the guts of how a object could be placed into a list or hash table can be hidden via having the Object maintain a key or two. The abstraction two patches later to be what amounts to a vir*DefPtr further illustrates things work. Once/if someone gets further down through the code - the existing "confusing" and "disparate" way that driver objects are handled all gets neatly hidden in an Object. The object code handles searches and traversals so that each driver doesn't have to do that. All the vir*obj modules need to do is provide a callback that does the filtering/decision of whether the object is included in a returned list (think NumOf, List, Export type functions). The requirement is a primaryKey must be defined - so we must be in at least one hash table or list (or whatever). The secondaryKey is optional to allow for a secondary lookup that doesn't require going through all the primaryKey's in order to find a secondary way to find something (modeled after domain device objects and UUID/Name lookups). I tried to avoid making too many comments due to previous negative feedback. Not every driver/vir*obj needs two (e.g. virNodeDevice is only defined by one the name and virInterface has a UUID, but can also have a MAC). Obviously you have a suggestion for a better mechanism - so I'm waiting to read what that is. I'm fairly certain you understand the ultimate goal. Let's try and get there. John

On 06/05/2017 10:46 AM, Peter Krempa wrote: So that makes your comments more understandable - thanks! While I can appreciate the "object"-ness of saying create an object with a uuid and name is preferred over create an object with two generic keys - the whole point of this object was to be generic. Most callers would use UUID as the primary key, but NodeDevice, Interface, and Volume would use use Name. The secondary key is usually Name, but then there's Interface that can use a MAC or perhaps Secret which could use the usageID. The NodeDevice doesn't have a secondary key and volume could use a "key" or "path" as a secondary key. FWIW: Although volume isn't currently done with objects, it can be easily adjusted with this type of abstraction. I can change to use UUID/Name, but I don't believe it'll be far better. I went with "primaryKey" and "secondaryKey" because it allows the consumer to decide what they are. I'm not sure what misuse and confusion could exist over an object that requires one "char *" and optionally allows a second "char *". It's an object and storage vessel to allow for a mechanism to provide 'generic' keys to/for virHashLookup. OTOH: Forcing UUID/Name could restrict things a bit too much for Interface, Secrets, and Volumes which don't have a UUID and thus wouldn't only be able to use the virHashLookup for one key as opposed to two. That means usage of virHashSearch instead. Implementation wise it doesn't really matter, but the consumer is restricted in how they can use the object unless of course they want to misuse one of the names to represent something else. E.G. secret could misuse name to be usageID, interface could misuse name to be MAC, and volume could misuse UUID to be key or path. In the the long run the purpose of the keys is to be two unique strings to be used as hash table keys. In the end though it's "only" a name... Still if you abstract the FindBy{UUID|Name| to be FindByObject using primaryKey or secondaryKey, then you reduce replication of the FindBy algorithms too. Using void * is just a design choice. Using an enum and union is just another way. However, changing to enum/typed means the object perhaps knows more than it should and I would think there would be typed accessors that don't guarantee anything more than void w/r/t misuse. Still I do see value in a union that knows "stuff" about @def when it comes to listing function callbacks (NumOf, GetNames, Export). Of course you end up with switch/case in what should be generic rather than allowing the consumer handle the details. fields amongst the various drivers/vir*obj modules. It's not acting upon the contents of the def as it doesn't care to know the various differences. Unless of course you want to see a "common" @def as well ;-). Sure, it doesn't necessarily follow how Ref and Lock/Unlock act directly upon "well defined" fields (int/virMutex), rather the new objects utilize new fields in order to allow the consumer act directly. Ok - well Poolable wasn't my favorite, but it just stuck while working through this. I also considered something with Hash or Table in it, but hash-able got me thinking too much about hash browns ;-) As for virEntity* vs. virObject* - I disagree with your suggestion. I refer back to the original RFC where it was suggested to essentially build upon the virObjectLockable and use "inheritance principles" rather than have a vir*ObjPrivate for each _vir*Obj that had driver/vir*obj specific fields to manage. Still if there's other opinions out there lurking - I am interesting in reading about them rather than hearing about them third hand. IMO: Your suggestion actually is closer to the initial RFC back in Jan/Feb which was rejected because it wasn't objectified using virObject. Rather than modifying src/util/ and virObject you'd rather see some new src/conf/* module. Right, the util code shouldn't know anything about Domain, Interface, etc., all it should care about is it's managing some object. It's a storage vessel that allows manipulation (access) to fields of the object so that the consumer can differentiate what a @def means to it. Right now a virLockableObject can act upon the @ref and @lock fields - they aren't the same type, but we've built up an API subsystem that makes use of them 'generically'. I'm merely extending that to use @primaryKey/@secondaryKey to Add, Find/Lookup/Search, and Remove generically from some listing object (in this case a hash table). An object that can also provide the virHashForEach generically by key in order to perform a callback passing the @obj to perform actions based on the def in the object (e.g. NumOf, GetName, and Export). The caller can decide whether type of List rather than being limited to UUID and Name. And yes, Export is going to be even more generic because it's providing a list of objects that each driver/vir*obj would know what call to make in order to fill in the table. Fair enough. And I post patches to garner feedback and ideas. I found posting as an RFC got no feedback, so shall I (ahem) assume that means no one cares or no one objects (bad pun, sorry). I welcome feedback and discussion. It doesn't necessarily have to be a dialog just between us either. If there's more opinions, let's hear them. I truly hope we can come to a group/list consensus. I'm flexible with the design within reasonable limits. John

Add a new virObjectPoolableHashElement child which will be used to provide the basis for driver def/newDef elements. Each virObjectPoolableDef has: 1. A required @primaryKey to be used to uniquely identity the object by some string value. 2. An optional @secondaryKey to be used as a secondary means of search for the object by some string value. 3. 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(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 24 ++++++++++++++ 3 files changed, 108 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd3ea68..1896f24 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; @@ -2270,6 +2271,7 @@ virObjectListFreeCount; virObjectLock; virObjectLockableNew; virObjectNew; +virObjectPoolableDefNew; virObjectPoolableHashElementGetPrimaryKey; virObjectPoolableHashElementGetSecondaryKey; virObjectPoolableHashElementNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 18cef0f..0aed790 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -63,9 +63,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) @@ -89,6 +91,13 @@ virObjectOnceInit(void) virObjectPoolableHashElementDispose))) return -1; + if (!(virObjectPoolableDefClass = + virClassNew(virObjectPoolableHashElementClass, + "virObjectPoolableDef", + sizeof(virObjectPoolableDef), + virObjectPoolableDefDispose))) + return -1; + return 0; } @@ -143,6 +152,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 @@ -334,6 +360,60 @@ virObjectPoolableHashElementDispose(void *anyobj) /** + * virObjectPoolableDefNew: + * @klass: the klass to check + * @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, + 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, 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 * @@ -400,7 +480,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 1ed856e..085ed43 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, @@ -69,10 +72,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) @@ -125,6 +140,15 @@ virObjectPoolableHashElementNew(virClassPtr klass, const char *secondaryKey) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +void * +virObjectPoolableDefNew(virClassPtr klass, + const char *primaryKey, + const char *secondaryKey, + void *def, + virFreeCallback defFreeFunc) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(5); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.9.4

On 06/05/2017 08:29 AM, Peter Krempa wrote: Right - semantics it's just a "next" possible configuration (domain, storage, network, and nwfilter have them)... Assume for a moment that the previous patches have the following: +struct _virObjectLookupKeys { + virObjectLockable parent; + + char *uuid; + char *name; +}; + and everything that goes with it. Essentially, exchange PoolableHashElement with LookupKeys. So this presents an interesting quandary. The goal is to have an object to manage essentially common things between all _vir*Obj structures; however, those common things are pointers to driver/object specific definition data. Still in the long run the object isn't *managing* the data it's merely acting as a storage vessel for common data allowing the consumer to handle the rest of the details. If I consider what you wrote it response to patch 5, there would be a union of sorts: union { virNodeDeviceDefPtr nodedev; virSecretDefPtr secret; virDomainDefPtr domain; virNetworkDefPtr network; virInterfaceDefPtr interface; virNWFilterDefPtr nwfilter; virStoragePoolDefPtr pool; virStorageVolDefPtr volume; } def; where def is placed into some new Object: struct _virObject???? { virObjectLookupKeys parent; virObject???Type deftype; union {} def; (8 types) virObject???Type newDeftype; union {} newDef; (only 4 types) }; That's all well and good, but the object code doesn't need nor does it want to manage anything w/r/t the specifics of the @def's. So the only reason to be able 'type' the @def would seem to be to have some sort of compile time safety check that (for example) networks aren't using domain object data. Even with the type'd @def/@newDef unions, unless there's going to be APIs in the object for each type of definition, how does one "compile time" set or get those objects other than using a #define as a wrapper for at least fetch. How set works in an API I don't have a mental picture yet beyond passing a @def as a "void *". Perhaps someone else has some brilliant idea. I suppose another option is 8 separate klasses for each of the various types of driver/vir*obj that would consume them. Still that seems overkill and unnecessary since the original object could store just as easily. The whole purpose of a single object is to store "something" that the consumer can use. That something would need a FreeFunc since we don't know what it is. I'm still preferential to the current model but perhaps add a @type parameter to the object and to the various get/set API's for some level of type checking, but I don't believe that's what's being asked for. Hopefully by responding again some conversation is generated. While you consider being generic going to far in one direction, I see typed values as no change from the current. Of course I have in mind about 8-10 steps after these patches - so I'm currently blinded by the chosen mechanism. Tks - John