[libvirt] [PATCH 1/7] qemu: Store inactive domain configuration in snapshot

[John Ferlan]

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
> Inorder to capture the exact state of domain, inactive configuration

In order

> is needed along with active configuration. This patch stores inactive
> domain configuration when creating snapshot of a running domain. It
> also captures the inactive snapshot configuration when a snapshot is
> redefined.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp at linux.vnet.ibm.com>
> ---
>  src/conf/snapshot_conf.c | 13 +++++++++++++
>  src/conf/snapshot_conf.h |  1 +
>  src/qemu/qemu_driver.c   | 10 ++++++++++
>  3 files changed, 24 insertions(+)
> 

Caveat emptor - I'm not 100% familiar with all/any of the snapshot_conf
code and processing, but I'll look at the patches to provide some
feedback...

Editorial comment... Using "newDef" has always been a bit "confusing" to
say the least.  I know at one point there was a comment on list in some
review that we should rename it to something else to indicate what it
really is, but I have forgotten what the recommendation was. Hopefully,
someone else can remember!

Perhaps we can "avoid" continuing the confusion with this set of
changes. Even more hopefully, someone spends the time to change domain
newDef ;-)!  That might even fix what I'm finding confusing about these
patches...

Looking at existing code prior to these changes and thinking about the
context of just these changes as they relate to def/newDef - I'm not
sure you're capturing "everything". For instance, I see code in
qemuDomainSnapshotDiskDataCollect makes specific decisions about which
disks to collect, but there's no adjustment in that code with this series.

There's a comment in qemuDomainRevertToSnapshot from a hunk of code just
above the !REVERT_ACTIVE_ONLY from patch 2:

"
     /* Prepare to copy the snapshot inactive xml as the config of this
      * domain.
      *
      * XXX Should domain snapshots track live xml rather
      * than inactive xml?  */
"

and from qemuDomainSnapshotCreateXML above where you add the newDef copy
in this patch:

"
         /* Easiest way to clone inactive portion of vm->def is via
          * conversion in and back out of xml.  */
"

So what's confusing (to say the least) is - which "def" is really being
used. The code at times says inactive, but IIUC newDef is generally
treated as the persistent def, but that only matters if something has
caused it to be created.  Of course, I assume this confusion is what
you're trying to fix based on the cover letter comments!

If the intention of these patches is to essentially "mimic" the domain
def/newDef functionality, then perhaps thinking in terms of something
like virDomainObjGetPersistentDef may be helpful to "hide" which is
which and when one applies over the other. (does that even make sense?)



> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index f0e852c..bfe3d6c 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
>          virDomainSnapshotDiskDefClear(&def->disks[i]);
>      VIR_FREE(def->disks);
>      virDomainDefFree(def->dom);
> +    virDomainDefFree(def->newDom);
>      virObjectUnref(def->cookie);
>      VIR_FREE(def);
>  }
> @@ -1336,6 +1337,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>              }
>          }
>  
> +        if (other->def->newDom) {
> +            if (def->newDom) {
> +                if (!virDomainDefCheckABIStability(other->def->newDom,
> +                                                   def->newDom, xmlopt))
> +                    goto cleanup;
> +            } else {
> +                /* Transfer the inactive domain def */
> +                def->newDom = other->def->newDom;
> +                other->def->newDom = NULL;

/* Steal the inactive domain def */
VIR_STEAL_PTR(def->newDom, other->def->newDom);

[yes, I realize this is just stealing the code a few lines above, but
the more recent desire is to use the macro]

> +            }
> +        }
> +
>          if (other == vm->current_snapshot) {
>              *update_current = true;
>              vm->current_snapshot = NULL;
> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
> index 1d663c7..0bc915f 100644
> --- a/src/conf/snapshot_conf.h
> +++ b/src/conf/snapshot_conf.h
> @@ -75,6 +75,7 @@ struct _virDomainSnapshotDef {
>      virDomainSnapshotDiskDef *disks;
>  
>      virDomainDefPtr dom;
> +    virDomainDefPtr newDom;
>  
>      virObjectPtr cookie;
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 74fdfdb..4ffec70 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15035,6 +15035,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                                                   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
>              goto endjob;
>  

IIUC, the above hunk is designed to grab from vm->def anything that was
felt to be in the domain def making it as if it was some sort of
inactive definition. But since it's copying the live definition it
perhaps copies portions of the live definition that have changed, such
as via hotplug. That leaves a couple of questions in my mind related to
whether restoration from a snapshot would/should "restore" that hotplug
element especially if it doesn't apply/exist any more.

The subsequent hunk essentially copies the persistent definition, so
does "PARSE_INACTIVE" make sense?

John

BTW: I'll respond individually to other patches, but it may take a few
hours as I have some errands to run today - so I'll be "in and out" -
figured it was better to get this out there first though.


> +        if (vm->newDef) {
> +            if (!(xml = qemuDomainDefFormatLive(driver, vm->newDef, priv->origCPU,
> +                                                true, true)) ||
> +                !(def->newDom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL,
> +                                                        VIR_DOMAIN_DEF_PARSE_INACTIVE |
> +                                                        VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
> +                goto endjob;
> +        }
> +
> +
>          if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
>              align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>              align_match = false;
>