Upcoming patches want to add virDomainCheckpoint that behaves very
similarly to virDomainSnapshot; the easiest way to share common code
is to give both classes a common base class.  Thanks to the accessor
functions in the previous patch, we have very few changes required
outside of datatypes.[ch].  The subclass does have to use a dummy
field, though, to satisfy virobject's insistence on size
differentiation for type safety.

Note that virClassNew() supports a NULL dispose method for a class
that has nothing to clean up, but VIR_CLASS_NEW has no easy way to
register such a class without a #define hack.

I promised my teenage daughter Evelyn that I'd give her credit for her
contribution to this commit. I asked her "What would be a good name
for a base class for DomainSnapshot and DomainCheckpoint". After
explaining what a base class was (using the classic OOB Square and
Circle inherit from Shape), she came up with "DomainMoment", which is
way better than my initial thought of "DomainPointInTime" or
"DomainPIT".

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/datatypes.h               |  26 ++++++--
 src/datatypes.c               | 110 ++++++++++++++++++++--------------
 src/libvirt-domain-snapshot.c |   2 +-
 3 files changed, 88 insertions(+), 50 deletions(-)

diff --git a/src/datatypes.h b/src/datatypes.h
index a66dfbe983..70d947657b 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -31,6 +31,7 @@

 extern virClassPtr virConnectClass;
 extern virClassPtr virDomainClass;
+extern virClassPtr virDomainMomentClass;
 extern virClassPtr virDomainSnapshotClass;
 extern virClassPtr virInterfaceClass;
 extern virClassPtr virNetworkClass;
@@ -668,27 +669,42 @@ struct _virStream {
     virFreeCallback ff;
 };

+/**
+ * _virDomainMoment
+ *
+ * Internal abstract structure serving as a base class to a named
+ * point in time object related to a domain
+ */
+typedef struct _virDomainMoment virDomainMoment;
+typedef virDomainMoment *virDomainMomentPtr;
+struct _virDomainMoment {
+    virObject parent;
+    char *name;
+    virDomainPtr domain;
+};
+
 /**
  * _virDomainSnapshot
  *
  * Internal structure associated with a domain snapshot
  */
 struct _virDomainSnapshot {
-    virObject parent;
-    char *name;
-    virDomainPtr domain;
+    virDomainMoment parent;
+
+    /* Unused attribute to allow for subclass creation */
+    bool dummy;
 };

 static inline char *
 virSnapName(virDomainSnapshotPtr snapshot)
 {
-    return snapshot->name;
+    return snapshot->parent.name;
 }

 static inline virDomainPtr
 virSnapDom(virDomainSnapshotPtr snapshot)
 {
-    return snapshot->domain;
+    return snapshot->parent.domain;
 }

 /**
diff --git a/src/datatypes.c b/src/datatypes.c
index 9b92d892d5..f0cfbe11fc 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -1,7 +1,7 @@
 /*
  * datatypes.c: management of structs for public data types
  *
- * Copyright (C) 2006-2015 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -36,6 +36,7 @@ VIR_LOG_INIT("datatypes");
 virClassPtr virConnectClass;
 virClassPtr virConnectCloseCallbackDataClass;
 virClassPtr virDomainClass;
+virClassPtr virDomainMomentClass;
 virClassPtr virDomainSnapshotClass;
 virClassPtr virInterfaceClass;
 virClassPtr virNetworkClass;
@@ -50,7 +51,8 @@ virClassPtr virStoragePoolClass;
 static void virConnectDispose(void *obj);
 static void virConnectCloseCallbackDataDispose(void *obj);
 static void virDomainDispose(void *obj);
-static void virDomainSnapshotDispose(void *obj);
+static void virDomainMomentDispose(void *obj);
+#define virDomainSnapshotDispose NULL
 static void virInterfaceDispose(void *obj);
 static void virNetworkDispose(void *obj);
 static void virNodeDeviceDispose(void *obj);
@@ -86,7 +88,8 @@ virDataTypesOnceInit(void)
     DECLARE_CLASS_LOCKABLE(virConnect);
     DECLARE_CLASS_LOCKABLE(virConnectCloseCallbackData);
     DECLARE_CLASS(virDomain);
-    DECLARE_CLASS(virDomainSnapshot);
+    DECLARE_CLASS(virDomainMoment);
+    DECLARE_CLASS_COMMON(virDomainSnapshot, virDomainMomentClass);
     DECLARE_CLASS(virInterface);
     DECLARE_CLASS(virNetwork);
     DECLARE_CLASS(virNodeDevice);
@@ -900,6 +903,64 @@ virNWFilterBindingDispose(void *obj)
 }


+/**
+ * virGetDomainMoment:
+ * @domain: the domain involved in a point-in-time moment
+ * @name: pointer to the domain moment name
+ *
+ * Allocates a new concrete subclass of a domain moment object. When
+ * the object is no longer needed, virObjectUnref() must be called in
+ * order to not leak data.
+ *
+ * Returns a pointer to the domain moment object, or NULL on error.
+ */
+static virDomainMomentPtr
+virGetDomainMoment(virDomainPtr domain, const char *name, virClassPtr subclass)
+{
+    virDomainMomentPtr ret = NULL;
+
+    if (virDataTypesInitialize() < 0)
+        return NULL;
+
+    virCheckDomainGoto(domain, error);
+    virCheckNonNullArgGoto(name, error);
+
+    if (!(ret = virObjectNew(subclass)))
+        goto error;
+    if (VIR_STRDUP(ret->name, name) < 0)
+        goto error;
+
+    ret->domain = virObjectRef(domain);
+
+    return ret;
+
+ error:
+    virObjectUnref(ret);
+    return NULL;
+}
+
+
+/**
+ * virDomainMomentDispose:
+ * @obj: the domain moment to release
+ *
+ * Unconditionally release all memory associated with a moment.
+ * The object must not be used once this method returns.
+ *
+ * It will also unreference the associated connection object,
+ * which may also be released if its ref count hits zero.
+ */
+static void
+virDomainMomentDispose(void *obj)
+{
+    virDomainMomentPtr moment = obj;
+    VIR_DEBUG("release moment %p %s", moment, moment->name);
+
+    VIR_FREE(moment->name);
+    virObjectUnref(moment->domain);
+}
+
+
 /**
  * virGetDomainSnapshot:
  * @domain: the domain to snapshot
@@ -913,47 +974,8 @@ virNWFilterBindingDispose(void *obj)
 virDomainSnapshotPtr
 virGetDomainSnapshot(virDomainPtr domain, const char *name)
 {
-    virDomainSnapshotPtr ret = NULL;
-
-    if (virDataTypesInitialize() < 0)
-        return NULL;
-
-    virCheckDomainGoto(domain, error);
-    virCheckNonNullArgGoto(name, error);
-
-    if (!(ret = virObjectNew(virDomainSnapshotClass)))
-        goto error;
-    if (VIR_STRDUP(ret->name, name) < 0)
-        goto error;
-
-    ret->domain = virObjectRef(domain);
-
-    return ret;
-
- error:
-    virObjectUnref(ret);
-    return NULL;
-}
-
-
-/**
- * virDomainSnapshotDispose:
- * @obj: the domain snapshot to release
- *
- * Unconditionally release all memory associated with a snapshot.
- * The snapshot object must not be used once this method returns.
- *
- * It will also unreference the associated connection object,
- * which may also be released if its ref count hits zero.
- */
-static void
-virDomainSnapshotDispose(void *obj)
-{
-    virDomainSnapshotPtr snapshot = obj;
-    VIR_DEBUG("release snapshot %p %s", snapshot, snapshot->name);
-
-    VIR_FREE(snapshot->name);
-    virObjectUnref(snapshot->domain);
+    return (virDomainSnapshotPtr) virGetDomainMoment(domain, name,
+                                                     virDomainSnapshotClass);
 }


diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index e1275c69b0..27fb350cc6 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -1206,7 +1206,7 @@ int
 virDomainSnapshotRef(virDomainSnapshotPtr snapshot)
 {
     VIR_DEBUG("snapshot=%p, refs=%d", snapshot,
-              snapshot ? snapshot->parent.u.s.refs : 0);
+              snapshot ? snapshot->parent.parent.u.s.refs : 0);

     virResetLastError();

-- 
2.20.1

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

On 3/20/19 1:40 AM, Eric Blake wrote:
> Upcoming patches want to add virDomainCheckpoint that behaves very
> similarly to virDomainSnapshot; the easiest way to share common code
> is to give both classes a common base class.  Thanks to the accessor
> functions in the previous patch, we have very few changes required
> outside of datatypes.[ch].  The subclass does have to use a dummy
> field, though, to satisfy virobject's insistence on size
> differentiation for type safety.
> 
> Note that virClassNew() supports a NULL dispose method for a class
> that has nothing to clean up, but VIR_CLASS_NEW has no easy way to
> register such a class without a #define hack.
> 
> I promised my teenage daughter Evelyn that I'd give her credit for her
> contribution to this commit. I asked her "What would be a good name
> for a base class for DomainSnapshot and DomainCheckpoint". After
> explaining what a base class was (using the classic OOB Square and
> Circle inherit from Shape), she came up with "DomainMoment", which is
> way better than my initial thought of "DomainPointInTime" or
> "DomainPIT".

Maybe it was her way to ensure some sort of "mom" 'ent'ry got included
into libvirt ;-}

> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/datatypes.h               |  26 ++++++--
>  src/datatypes.c               | 110 ++++++++++++++++++++--------------
>  src/libvirt-domain-snapshot.c |   2 +-
>  3 files changed, 88 insertions(+), 50 deletions(-)
> 

[...]

> 
> 
> +/**
> + * virGetDomainMoment:
> + * @domain: the domain involved in a point-in-time moment
> + * @name: pointer to the domain moment name

 * @subclass: Either virDomainSnapshotClass or virDomainCheckpointClass

[ok that second one is eventually.... and it's only internal so
validation is left to the developer ;-}]

> + *
> + * Allocates a new concrete subclass of a domain moment object. When
> + * the object is no longer needed, virObjectUnref() must be called in
> + * order to not leak data.
> + *
> + * Returns a pointer to the domain moment object, or NULL on error.
> + */
> +static virDomainMomentPtr
> +virGetDomainMoment(virDomainPtr domain, const char *name, virClassPtr subclass)
> +{
> +    virDomainMomentPtr ret = NULL;
> +
> +    if (virDataTypesInitialize() < 0)
> +        return NULL;
> +
> +    virCheckDomainGoto(domain, error);
> +    virCheckNonNullArgGoto(name, error);
> +
> +    if (!(ret = virObjectNew(subclass)))
> +        goto error;
> +    if (VIR_STRDUP(ret->name, name) < 0)
> +        goto error;
> +
> +    ret->domain = virObjectRef(domain);
> +
> +    return ret;
> +
> + error:
> +    virObjectUnref(ret);
> +    return NULL;
> +}
> +
> +

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

I have to say I did pause to stop and think about the Dispose thing...
Especially with the "parent" class having a NULL ->dispose, but also
having a klass->parent which would have the MomentDispose.


[...]

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

On 3/20/19 2:57 PM, John Ferlan wrote:
> 
> 
> On 3/20/19 1:40 AM, Eric Blake wrote:
>> Upcoming patches want to add virDomainCheckpoint that behaves very
>> similarly to virDomainSnapshot; the easiest way to share common code
>> is to give both classes a common base class.  Thanks to the accessor
>> functions in the previous patch, we have very few changes required
>> outside of datatypes.[ch].  The subclass does have to use a dummy
>> field, though, to satisfy virobject's insistence on size
>> differentiation for type safety.

When I first wrote this patch, I was envisioning a common function in
virdomainmomentobjlist.c that would take a virClassPtr() argument and
spit back an appropriate list of subclassed objects but with a function
signature using virDomainMomentPtr **list (which I could then down-cast
to virDomainSnapshotPtr ** or virDomainCheckpointPtr ** as appropriate).
 However, in re-reading what I actually did in patch 14/16, I ended up
keeping virDomainListSnapshots() as-is rather than moving it into
virdomainmomentobjlist.c; it turned out that
virDomainMomentObjListNames() with its char **list parameter was all the
more generic code I needed.  As such, I'm not seeing any use in my
current code that needs the virDomainMoment type, so I'll probably drop
patches 2 and 3 and rework 16/16 to have virDomainCheckpoint subclass
virObject rather than virDomainMoment.

The subclassing of virDomainMomentDef and virDomainMomentObjList (the
rest of this series) still makes sense, though, and I'm also looking at
converting those types to use virObject rather than being bare C structs
(but it would be followup patches, not holding up the rest of this
series from going in according to reviews).

>>
>> Note that virClassNew() supports a NULL dispose method for a class
>> that has nothing to clean up, but VIR_CLASS_NEW has no easy way to
>> register such a class without a #define hack.
>>
>> I promised my teenage daughter Evelyn that I'd give her credit for her
>> contribution to this commit. I asked her "What would be a good name
>> for a base class for DomainSnapshot and DomainCheckpoint". After
>> explaining what a base class was (using the classic OOB Square and
>> Circle inherit from Shape), she came up with "DomainMoment", which is
>> way better than my initial thought of "DomainPointInTime" or
>> "DomainPIT".
> 
> Maybe it was her way to ensure some sort of "mom" 'ent'ry got included
> into libvirt ;-}

Of course, if I drop this commit from my series, I'll have to move the
historical note into 10/16 (as the first use of that prefix after this
patch).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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