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

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd3ea68..1896f24 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2258,6 +2258,7 @@ virNumaSetupMemoryPolicy;
 # util/virobject.h
 virClassForObject;
 virClassForObjectLockable;
+virClassForObjectPoolableDef;
 virClassForObjectPoolableHashElement;
 virClassIsDerivedFrom;
 virClassName;
@@ -2270,6 +2271,7 @@ virObjectListFreeCount;
 virObjectLock;
 virObjectLockableNew;
 virObjectNew;
+virObjectPoolableDefNew;
 virObjectPoolableHashElementGetPrimaryKey;
 virObjectPoolableHashElementGetSecondaryKey;
 virObjectPoolableHashElementNew;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 18cef0f..0aed790 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -63,9 +63,11 @@ struct _virClass {
 static virClassPtr virObjectClass;
 static virClassPtr virObjectLockableClass;
 static virClassPtr virObjectPoolableHashElementClass;
+static virClassPtr virObjectPoolableDefClass;
 
 static void virObjectLockableDispose(void *anyobj);
 static void virObjectPoolableHashElementDispose(void *anyobj);
+static void virObjectPoolableDefDispose(void *anyobj);
 
 static int
 virObjectOnceInit(void)
@@ -89,6 +91,13 @@ virObjectOnceInit(void)
                       virObjectPoolableHashElementDispose)))
         return -1;
 
+    if (!(virObjectPoolableDefClass =
+          virClassNew(virObjectPoolableHashElementClass,
+                      "virObjectPoolableDef",
+                      sizeof(virObjectPoolableDef),
+                      virObjectPoolableDefDispose)))
+        return -1;
+
     return 0;
 }
 
@@ -143,6 +152,23 @@ virClassForObjectPoolableHashElement(void)
 
 
 /**
+ * virClassForObjectPoolableDef:
+ *
+ * Returns the class instance for the virObjectPoolableDef type
+ */
+virClassPtr
+virClassForObjectPoolableDef(void)
+{
+    if (virObjectInitialize() < 0)
+        return NULL;
+
+    VIR_DEBUG("virObjectPoolableDefClass=%p",
+              virObjectPoolableDefClass);
+    return virObjectPoolableDefClass;
+}
+
+
+/**
  * virClassNew:
  * @parent: the parent class
  * @name: the class name
@@ -334,6 +360,60 @@ virObjectPoolableHashElementDispose(void *anyobj)
 
 
 /**
+ * virObjectPoolableDefNew:
+ * @klass: the klass to check
+ * @primaryKey: primary key (required)
+ * @secondaryKey: secondary key
+ * @def: XML definition (required)
+ * @defFreeFunc: Free function for @def and @newDef (required)
+ *
+ * Create a new poolable def object for storing "common" domain defs.
+ *
+ * Returns: New object on success, NULL on failure w/ error message set
+ */
+void *
+virObjectPoolableDefNew(virClassPtr klass,
+                        const char *primaryKey,
+                        const char *secondaryKey,
+                        void *def,
+                        virFreeCallback defFreeFunc)
+{
+    virObjectPoolableDefPtr obj;
+
+    if (!virClassIsDerivedFrom(klass, virClassForObjectPoolableDef())) {
+        virReportInvalidArg(klass,
+                            _("Class %s must derive from "
+                              "virObjectPoolableDef"),
+                            virClassName(klass));
+        return NULL;
+    }
+
+    if (!(obj = virObjectPoolableHashElementNew(klass, primaryKey,
+                                                secondaryKey)))
+        return NULL;
+
+    obj->def = def;
+    obj->defFreeFunc = defFreeFunc;
+
+    VIR_DEBUG("obj=%p, def=%p ff=%p", obj, obj->def, obj->defFreeFunc);
+
+    return obj;
+}
+
+
+static void
+virObjectPoolableDefDispose(void *anyobj)
+{
+    virObjectPoolableDefPtr obj = anyobj;
+
+    VIR_DEBUG("dispose obj=%p", obj);
+
+    (obj->defFreeFunc)(obj->def);
+    (obj->defFreeFunc)(obj->newDef);
+}
+
+
+/**
  * virObjectUnref:
  * @anyobj: any instance of virObjectPtr
  *
@@ -400,7 +480,8 @@ static virObjectLockablePtr
 virObjectGetLockableObj(void *anyobj)
 {
     if (virObjectIsClass(anyobj, virObjectLockableClass) ||
-        virObjectIsClass(anyobj, virObjectPoolableHashElementClass))
+        virObjectIsClass(anyobj, virObjectPoolableHashElementClass) ||
+        virObjectIsClass(anyobj, virObjectPoolableDefClass))
         return anyobj;
 
     VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, virObjectLockableClass);
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 1ed856e..085ed43 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -37,6 +37,9 @@ typedef virObjectLockable *virObjectLockablePtr;
 typedef struct _virObjectPoolableHashElement virObjectPoolableHashElement;
 typedef virObjectPoolableHashElement *virObjectPoolableHashElementPtr;
 
+typedef struct _virObjectPoolableDef virObjectPoolableDef;
+typedef virObjectPoolableDef *virObjectPoolableDefPtr;
+
 typedef void (*virObjectDisposeCallback)(void *obj);
 
 /* Most code should not play with the contents of this struct; however,
@@ -69,10 +72,22 @@ struct _virObjectPoolableHashElement {
     char *secondaryKey;
 };
 
+struct _virObjectPoolableDef {
+    virObjectPoolableHashElement parent;
+
+    /* 'def' is the current config definition.
+     * 'newDef' is the next boot configuration.
+     */
+    void *def;
+    void *newDef;
+    virFreeCallback defFreeFunc;
+};
+
 
 virClassPtr virClassForObject(void);
 virClassPtr virClassForObjectLockable(void);
 virClassPtr virClassForObjectPoolableHashElement(void);
+virClassPtr virClassForObjectPoolableDef(void);
 
 # ifndef VIR_PARENT_REQUIRED
 #  define VIR_PARENT_REQUIRED ATTRIBUTE_NONNULL(1)
@@ -125,6 +140,15 @@ virObjectPoolableHashElementNew(virClassPtr klass,
                                 const char *secondaryKey)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
+void *
+virObjectPoolableDefNew(virClassPtr klass,
+                        const char *primaryKey,
+                        const char *secondaryKey,
+                        void *def,
+                        virFreeCallback defFreeFunc)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
+    ATTRIBUTE_NONNULL(5);
+
 void
 virObjectLock(void *lockableobj)
     ATTRIBUTE_NONNULL(1);
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.

> +     */
> +    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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list