[libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine

[John Ferlan]

On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
> This patch just adds basic checks for persistent domain config
> on snapshot metadata redefine. It also lets use previous version
> of config if it exists in previous version of metadata and
> not explicitly specified in new one just as in case of top level
> config.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 

Should this actually be part of patch5? Although nice to have patches
separated for review - functionality wise it's paired with managing the
persistDom for the snapshot_conf.


> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index fa1cd6b..b003a07 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>          goto cleanup;
>      }
>  
> +    if (def->persistDom &&
> +        memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("persistent definition for snapshot %s must use uuid %s"),
> +                       def->name, uuidstr);
> +        goto cleanup;
> +    }
> +
> +    if (def->persistDom &&
> +        STRNEQ(def->persistDom->name, domain->name)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("persistent definition for snapshot %s must use name %s"),
> +                       def->name, domain->name);
> +        goto cleanup;
> +    }
> +

Could go with the:

    if (def->persistDom) {
    }

here too, but IDC that much.

Based on any fallout from other comments received on patch7 and since
this essentially "copies" things for the persistDom,

>      other = virDomainSnapshotFindByName(vm->snapshots, def->name);
>      if (other) {
>          if ((other->def->state == VIR_DOMAIN_RUNNING ||
> @@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>              }
>          }
>  
> +        if (other->def->persistDom && !def->persistDom) {
> +            def->persistDom = other->def->persistDom;
> +            other->def->persistDom = NULL;
> +        }

Isn't this just

    VIR_STEAL_PTR(def->persistDom, other->def->persistDom);

?

> +
>          if (def->dom) {
>              if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
>                  virDomainSnapshotDefIsExternal(def)) {
> @@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
>                      other->def->dom = def->dom;
>                      def->dom = NULL;
>                  }
> +                if (def->persistDom && !other->def->persistDom) {
> +                    other->def->persistDom = def->persistDom;
> +                    def->persistDom = NULL;
> +                }

Likewise...

>                  goto cleanup;
>              }
>          }
> 

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

John