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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
