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

Mon Jun 5 14:46:34 UTC 2017

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.

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.

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.

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

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.

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170605/dbb572c6/attachment-0001.sig>