[libvirt] [PATCH v2 7/8] util: Introduce virObjectPoolableDef

[John Ferlan]

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