On 06/05/2017 03:39 AM, Peter Krempa wrote:

On Fri, Jun 02, 2017 at 06:17:17 -0400, John Ferlan wrote:

...

If virObjectIsClass fails "internally" to virobject.c, create a macro to generate the VIR_WARN describing what the problem is. Also improve the checks and message a bit to indicate which was the failure - whether the obj was NULL or just not the right class

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virobject.c | 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);\

Is this really helpful?

Object (nil) is not a virObject class instance.

%p does not make sense since it's always NULL, and also obviously a null pointer is not a class instance ...

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:

The virObject logic "assumes" that whatever is passed to its API's would be some sort of virObjectPtr; however, if it is not then some really bad things can happen.

So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock}, and virObjectIsClass and the virObject and virObjectLockable class consumers have been well behaved and code well tested. Soon there will be more consumers and one such consumer tripped over this during testing by passing a virHashTablePtr to virObjectIsClass which ends up calling virClassIsDerivedFrom using "obj->klass", which wasn't really a klass object causing one of those bad things to happen.

To avoid the future possibility that a non virObject class memory was passed to some virObject* API, this patch adds two new checks - one to validate that the object has the 0xCAFExxxx value in obj->->u.s.magic and the other to ensure obj->u.s.magic doesn't "wrap" some day to 0xCAFF0000 if we ever get that many objects.

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 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/util/virobject.c b/src/util/virobject.c index 9f5f187..e0465b1 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c

[..]

@@ -156,6 +159,7 @@ virClassNew(virClassPtr parent, if (VIR_STRDUP(klass->name, name) < 0) goto error; klass->magic = virAtomicIntInc(&magicCounter); + assert(klass->magic <= 0xCAFEFFFF);

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.

On 06/05/2017 08:25 AM, Peter Krempa wrote:

On Fri, Jun 02, 2017 at 06:17:19 -0400, John Ferlan wrote:

...

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@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 17 +++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-)

[...]

...

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

[...]

...

@@ -59,9 +62,17 @@ struct _virObjectLockable { virMutex lock; };

+struct _virObjectPoolableHashElement { + virObjectLockable parent; + + char *primaryKey; + char *secondaryKey; +};

I'm afraid that this abstraction is going too far.

Putting dissimilar objects into a single hash does not really make sense in any way in libvirt. Without the need to put dissimilar objects into a single hash I don't really see value in abstracting the identifiers of the objects into opaque things like 'primaryKey'.

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

Refering to the objects by these oaque terms will cause confusion, and thus will still require wrappers to de-confuse readers of the code.

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).

An additional worry is the optionality of the secondary key. This hints that the objects are so dissimilar that they don't have two names in common.

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:

On Mon, Jun 05, 2017 at 09:10:00 -0400, John Ferlan wrote:

...

On 06/05/2017 08:25 AM, Peter Krempa wrote:

...

On Fri, Jun 02, 2017 at 06:17:19 -0400, John Ferlan wrote:

...

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@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 17 +++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-)

[...]

...

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

[...]

...

@@ -59,9 +62,17 @@ struct _virObjectLockable { virMutex lock; };

+struct _virObjectPoolableHashElement { + virObjectLockable parent; + + char *primaryKey; + char *secondaryKey; +};

I'm afraid that this abstraction is going too far.

Putting dissimilar objects into a single hash does not really make sense in any way in libvirt. Without the need to put dissimilar objects into a single hash I don't really see value in abstracting the identifiers of the objects into opaque things like 'primaryKey'.

They're not in a single hash table. Honestly, your comment makes no

Okay, that would be insane.

...

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

...

Refering to the objects by these oaque terms will cause confusion, and thus will still require wrappers to de-confuse readers of the code.

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.

This is fine. What I consider too generic is that you have the same kind of object for VMs and interfaces and other things. You then need to use a void pointer to access definition. That kind of abstraction went too far. This is far better done by adding a separate class without use of any opaque pointers.

Same thing with the primaryKey and secondaryKey. Since you can't give them a specific name (like UUID) you've are abstracting them too much.

You still need functions that wrap all this infrastructure so that type-safe accessors are provided. Also you need wrappers to provide sane names for primaryKey and secondaryKey, this means that it's again too abstract, since the data by itself is not meaningful.

...

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).

...

An additional worry is the optionality of the secondary key. This hints that the objects are so dissimilar that they don't have two names in common.

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).

The problem is that those are too generic. This is the abstraction I think went too far.

If you make an object which abstracts:

char *name; char *uuid;

this will be far better. Both are reasonably generic and you don't get the opaqueness of 'primaryKey' which can mean everything. Taking the abstraction too far will lead to misuse and confusion. Also it will still require having wrapper functions for every single kind of object you want to store in it to translate it back to name or UUID. Using the 'primaryKey' by itself will only induce problems since it is valid only in a certain context.

Yes, that makes both of them optional in certain cases, but then you need to reconsider whether it's worth abstracting them in that case.

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.

If you can't assign names to the things you are splitting out, because they differ among the classes, you should not try to abstract them.

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.

Similarly using void * for the definition is going too far. Since you still need to add function to add/remove/whatever elements from the list/hash which will take the precise type of the objects (no, having only functions which take void * is not good enough since you don't have type checking.)

You can aggregate the pointers by using a discriminator enum and a union. Since you need accessors anyways, this will remove the need for void pointers in the object itself. It will also make people reconsider reusing this in unexpected ways.

From my viewpoint, the object is essentially a storage vessel for common fields amongst the various drivers/vir*obj modules. It's not acting upon

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. 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.

...

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.

I don't opose to unify the listing and storing of objects. I'm just oposing taking the abstraction too far. Also I don't quite like the "Poolable" name. If you remove the "Object" part which is reserved for virObjects and clases and do a virEntityList and virEntityEntry (or something similar) you disconnect the thing from objects ... they just use objects, but are way more specific.

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.

This will be emphasised by the fact that you can't really access virDomainObjPtr, virInterfaceObjPtr, ... from util, so this will need to live in some other place.

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.

In general, I don't oppose a infrastructure that will agregate libvirt's entity objects. I just don't want us to go too abstract, since that will atract mistakes and also misuse.

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

On 06/05/2017 08:29 AM, Peter Krempa wrote:

On Fri, Jun 02, 2017 at 06:17:21 -0400, John Ferlan wrote:

...

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@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virobject.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++- src/util/virobject.h | 24 ++++++++++++++ 3 files changed, 108 insertions(+), 1 deletion(-)

[...]

...

@@ -69,10 +72,22 @@ struct _virObjectPoolableHashElement { char *secondaryKey; };

+struct _virObjectPoolableDef { + virObjectPoolableHashElement parent; + + /* 'def' is the current config definition. + * 'newDef' is the next boot configuration.

'boot' does not really apply to anything besides VMs.

Right - semantics it's just a "next" possible configuration (domain, storage, network, and nwfilter have them)...

...

+ */ + void *def; + void *newDef; + virFreeCallback defFreeFunc; +};

Okay, this is another sign that this abstraction has gone too far. Using void pointers here for the definition pointers really does not help stuff. You need wrappers to do compile time type safety checks here, so I don't really see the value to wrap it into a object with such opaqueness.

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