On 21.12.2018 00:15, John Ferlan wrote:
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@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;
Yeah, a bit of copy-paste. In this case I would like to add patch to use VIR_STEAL_PTR for existing code in virDomainSnapshotRedefinePrep too. Nikolay
} }
Reviewed-by: John Ferlan <jferlan@redhat.com>
John