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>
---
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>
John