[libvirt] [PATCH 10/16] snapshot: Factor out virDomainMomentDef class

[John Ferlan]

On 3/20/19 1:40 AM, Eric Blake wrote:
> Pull out the common parts of virDomainSnapshotDef that will be reused
> for virDomainCheckpointDef into a new base class.  Adjust all callers
> that use the direct fields (some of it is churn that disappears when
> the next patch refactors virDomainSnapshotObj; oh well...).
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/moment_conf.h              |  41 +++++++++
>  src/conf/snapshot_conf.h            |  11 +--
>  src/conf/virconftypes.h             |   3 +
>  src/conf/Makefile.inc.am            |   2 +
>  src/conf/moment_conf.c              |  40 +++++++++
>  src/conf/snapshot_conf.c            | 128 ++++++++++++++--------------
>  src/conf/virdomainsnapshotobj.c     |   2 +-
>  src/conf/virdomainsnapshotobjlist.c |  21 ++---
>  src/esx/esx_driver.c                |  16 ++--
>  src/qemu/qemu_command.c             |   2 +-
>  src/qemu/qemu_domain.c              |  18 ++--
>  src/qemu/qemu_driver.c              |  50 +++++------
>  src/test/test_driver.c              |  42 ++++-----
>  src/vbox/vbox_common.c              |  88 +++++++++----------
>  14 files changed, 273 insertions(+), 191 deletions(-)
>  create mode 100644 src/conf/moment_conf.h
>  create mode 100644 src/conf/moment_conf.c
> 

So since I'm at the magical patch mentioned in your updates to patch 3
where you may need to move comments and attribution around... That's
fine by me. I understand this is all kind of a "dynamic state of
affairs".  While I understand your reasoning, having the example you
created for future consumption may not be a bad thing after all. One of
the things I struggled with a lot when making common objects was this
notion that many objects and in particular list objects are very similar
copies. If there was a parent class with varying subclasses that could
have their own particular interesting/necessary piece that actually
would be "more like" object code that I was driving towards. Long winded
way of saying - whatever you choose is fine, I just worry that at some
point in the future we may need to travel this road again and you've
already "solved" the problem, so let's just go with it. It wouldn't be
the first time...


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

John

Also - just a couple of things for follow-ups...

[...]

> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index aec23f111c..b80b199ce4 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c

[...]

> @@ -474,24 +472,24 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def,
>              virReportError(VIR_ERR_INVALID_ARG,
>                             _("cannot change between disk only and "
>                               "full system in snapshot %s"),
> -                           def->name);
> +                           def->common.name);
>              return -1;
>          }
> 
> -        if (otherdef->dom) {
> -            if (def->dom) {
> -                if (!virDomainDefCheckABIStability(otherdef->dom,
> -                                                   def->dom, xmlopt))
> +        if (otherdef->common.dom) {
> +            if (def->common.dom) {
> +                if (!virDomainDefCheckABIStability(otherdef->common.dom,
> +                                                   def->common.dom, xmlopt))
>                      return -1;
>              } else {
>                  /* Transfer the domain def */
> -                def->dom = otherdef->dom;
> -                otherdef->dom = NULL;
> +                def->common.dom = otherdef->common.dom;
> +                otherdef->common.dom = NULL;

Oh look a VIR_STEAL_PTR opportunity - for a trivial followup patch.

>              }
>          }
>      }
> 
> -    if (def->dom) {
> +    if (def->common.dom) {
>          if (external) {
>              align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>              align_match = false;

[...]

> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index 1c6a2dcb71..5cf881ae35 100644

[...]

> @@ -4189,12 +4189,12 @@ esxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
>          goto cleanup;
>      }
> 
> -    def.name = virSnapName(snapshot);
> -    def.description = snapshotTree->description;
> -    def.parent = snapshotTreeParent ? snapshotTreeParent->name : NULL;
> +    def.common.name = virSnapName(snapshot);
> +    def.common.description = snapshotTree->description;
> +    def.common.parent = snapshotTreeParent ? snapshotTreeParent->name : NULL;

/me thinking about this existing disaster w/ dual ownership of these
fields - who wins the race to free-dom?  I didn't chase *any* of this,
but it certainly doesn't look nice to making a copy of the pointer
rather than the string.

> 
>      if (esxVI_DateTime_ConvertToCalendarTime(snapshotTree->createTime,
> -                                             &def.creationTime) < 0) {
> +                                             &def.common.creationTime) < 0) {
>          goto cleanup;
>      }
> 
[...]