[libvirt] [PATCH v2 5/8] util: Introduce virObjectPoolableHashElement

[John Ferlan]

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