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

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(a)virtuozzo.com&gt; > --- > 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(a)redhat.com&gt; John