[libvirt] [PATCH 2/7] qemu: Use active and inactive snapshot configuration on restore

[John Ferlan]

On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
> By default, active and inactive XMl snapshot configurations are

*XML

> assigned to domain definition. This will make sure that all the
> non-persistent configurations of the snapshot are restored back
> as it is. This patch will also make sure that user has a choice

s/it is/they were/

??

> to choose of using active XML configuration of snapshot as both
> active and inactive XML configurations of the restoring domain.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp at linux.vnet.ibm.com>
> ---
>  include/libvirt/libvirt-domain-snapshot.h | 10 +++++++---
>  src/qemu/qemu_driver.c                    | 20 +++++++++++++++++++-
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
> index 0f73f24..67ccb59 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -184,9 +184,13 @@ int virDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot,
>                                   unsigned int flags);
>  
>  typedef enum {
> -    VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
> -    VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED  = 1 << 1, /* Pause after revert */
> -    VIR_DOMAIN_SNAPSHOT_REVERT_FORCE   = 1 << 2, /* Allow risky reverts */
> +    VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING       = 1 << 0, /* Run after revert */
> +    VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED        = 1 << 1, /* Pause after revert */
> +    VIR_DOMAIN_SNAPSHOT_REVERT_FORCE         = 1 << 2, /* Allow risky reverts */
> +    VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY   = 1 << 3, /* Use active snapshot
> +                                                          configurations as both
> +                                                          active and inactive
> +                                                          domain configurations*/

Previous def only had 1 space between longest line of "RUNNING" and "=";
this has 2 spaces between "ONLY" and "=".  Just go with 1.

Also the tailing "*/" should gain a space.

The comment doesn't quite make sense though related to the name used for
the enum. I'm having trouble understanding the meaning of "as both
active and inactive" as it relates to REVERT_ACTIVE_ONLY.

I think you're trying to indicate that by setting this flag, when
reverting from the snapshot, that only the "saved" active definition
would be restored and if there was a saved persistent one, then it would
be skipped.  IOW: "VIR_DOMAIN_SNAPSHOT_REVERT_SKIP_PERSISTENT"


>  } virDomainSnapshotRevertFlags;
>  
>  /* Revert the domain to a point-in-time snapshot.  The
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4ffec70..aecfcff 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15577,6 +15577,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>      qemuDomainObjPrivatePtr priv;
>      int rc;
>      virDomainDefPtr config = NULL;
> +    virDomainDefPtr newConfig = NULL;

persistentConfig?

>      virQEMUDriverConfigPtr cfg = NULL;
>      virCapsPtr caps = NULL;
>      bool was_running = false;
> @@ -15586,7 +15587,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>  
>      virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>                    VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
> -                  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);
> +                  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE |
> +                  VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY, -1);
>  
>      /* We have the following transitions, which create the following events:
>       * 1. inactive -> inactive: none
> @@ -15688,6 +15690,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>              goto endjob;
>      }
>  

This is why I ended up writing that long comment in patch 1.

> +    /* Prepare to copy snapshot inactive xml as inactive configuration
> +     * of this domain unless user exclusively specify not to copy it */
> +    if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY) &&
> +        snap->def->newDom) {
> +        newConfig = virDomainDefCopy(snap->def->newDom, caps,
> +                                     driver->xmlopt, NULL, true);
> +        if (!newConfig)
> +            goto endjob;
> +    }
> +
>      cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
>  
>      switch ((virDomainState) snap->def->state) {
> @@ -15785,12 +15797,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                  virCPUDefFree(priv->origCPU);
>                  VIR_STEAL_PTR(priv->origCPU, origCPU);
>              }
> +            if (newConfig)
> +                vm->newDef = newConfig;
>          } else {
>              /* Transitions 2, 3 */
>          load:
>              was_stopped = true;
>              if (config)
>                  virDomainObjAssignDef(vm, config, false, NULL);
> +            if (newConfig)
> +                vm->newDef = newConfig;

>  
>              /* No cookie means libvirt which saved the domain was too old to
>               * mess up the CPU definitions.
> @@ -15884,6 +15900,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>          }
>          if (config)
>              virDomainObjAssignDef(vm, config, false, NULL);
> +        if (newConfig)
> +            vm->newDef = newConfig;
>  


Shall I assume you're sure that for all assignments that vm->newDef
isn't already populated with something?

This is why I started thinking about the various existing convenience
API's that managed the def/newDef.


John
>          if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
>