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

[John Ferlan]

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

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.

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