2:03 a.m.
This series address the same issue as seemingly abandomed patch series [1].
Implementation is written from scratch however as I don't see need to introduce
new flags for the purpose of this patch series. However I read John's comments
and addressed them where applied. The option for not reverting inactive config
in case of restoring active persistent domain that Jiri suggested in [2] is new
functionality actually and is left for future work.
The purpuse of patch series is described in patch 3
"qemu: snapshot: save/restore inactive persistent config":
In case of active persistent domain snapshot metadata is
not complete. We save only active configuration and on
restore use it both for active and inactive configuration.
Let's fix it and save and restore both in this case.
[1] [PATCH 0/7] Keep non-persistent changes alive in snapshot
https://www.redhat.com/archives/libvir-list/2017-October/msg01333.html
[2] [PATCH] qemu: snapshot: Keep non-persistent changes alive in snapshot
https://www.redhat.com/archives/libvir-list/2017-October/msg01275.html
Nikolay Shirokovskiy (9):
qemu: release job on cleanup path
qemu: snapshot revert: reuse common cleanup code
qemu: snapshot: save/restore inactive persistent config
schema: snapshot: add persistent domain config
conf: snapshot: dump/parse persistent domain config
docs: add persistent config to snapshot xml description
conf: snapshot: check domain name on redefine
conf: snapshot: support persistent config on redefine
news: add notice for persistent config in snapshot
docs/formatsnapshot.html.in | 7 ++
docs/news.xml | 11 +++
docs/schemas/domainsnapshot.rng | 10 +++
src/conf/snapshot_conf.c | 60 +++++++++++++++
src/conf/snapshot_conf.h | 2 +
src/qemu/qemu_driver.c | 52 ++++++++++---
.../active_inactive_domain.xml | 85 ++++++++++++++++++++++
tests/domainsnapshotxml2xmltest.c | 1 +
8 files changed, 218 insertions(+), 10 deletions(-)
create mode 100644 tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
--
1.8.3.1
2:03 a.m.
New subject: [libvirt] [PATCH 1/9] qemu: release job on cleanup path
Introduced-by: 6a6f6b91e qemu: "Fix CPU model broken by older libvirt"
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 90cbc02..3242a60 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16457,7 +16457,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
*/
if (cookie &&
qemuDomainFixupCPUs(vm, &cookie->cpu) < 0)
- goto cleanup;
+ goto endjob;
rc = qemuProcessStart(snapshot->domain->conn, driver, vm,
cookie ? cookie->cpu : NULL,
--
1.8.3.1
2:26 p.m.
New subject: [libvirt] [PATCH 1/9] qemu: release job on cleanup path
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Introduced-by: 6a6f6b91e qemu: "Fix CPU model broken by older
libvirt"
Add a blank line here...
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
This is separate from the "core" of this and could be pushed without
other changes.
2:03 a.m.
New subject: [libvirt] [PATCH 2/9] qemu: snapshot revert: reuse common cleanup code
Commit [1] changed cleanup code to first remove inactive domain
and then release job so we can reuse common cleanup code.
[1] 9115dcd83 qemu: Introduce and use qemuDomainRemoveInactiveJob
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3242a60..f7c1d6f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16533,8 +16533,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) {
qemuDomainRemoveInactive(driver, vm);
- qemuProcessEndJob(driver, vm);
- goto cleanup;
+ goto endjob;
}
if (config)
virDomainObjAssignDef(vm, config, false, NULL);
@@ -16554,8 +16553,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virDomainAuditStart(vm, "from-snapshot", rc >= 0);
if (rc < 0) {
qemuDomainRemoveInactive(driver, vm);
- qemuProcessEndJob(driver, vm);
- goto cleanup;
+ goto endjob;
}
detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT;
event = virDomainEventLifecycleNewFromObj(vm,
--
1.8.3.1
2:27 p.m.
New subject: [libvirt] [PATCH 2/9] qemu: snapshot revert: reuse common cleanup code
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Commit [1] changed cleanup code to first remove inactive domain
and then release job so we can reuse common cleanup code.
[1] 9115dcd83 qemu: Introduce and use qemuDomainRemoveInactiveJob
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Similarly separable patch that could be pushed now.
2:03 a.m.
New subject: [libvirt] [PATCH 3/9] qemu: snapshot: save/restore inactive persistent config
In case of active persistent domain snapshot metadata is
not complete. We save only active configuration and on
restore use it both for active and inactive configuration.
Let's fix it and save and restore both in this case.
In case of active transient or inactive domain we have
only one config and thus everything is good.
By the way this patch fixes config memleak on error paths.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 1 +
src/conf/snapshot_conf.h | 2 ++
src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++++++++++++-----
3 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 5a511b4..2e4a0b9 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
virDomainSnapshotDiskDefClear(&def->disks[i]);
VIR_FREE(def->disks);
virDomainDefFree(def->dom);
+ virDomainDefFree(def->persistDom);
virObjectUnref(def->cookie);
VIR_FREE(def);
}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 20a42bd..3da204a 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -75,6 +75,8 @@ struct _virDomainSnapshotDef {
virDomainSnapshotDiskDef *disks;
virDomainDefPtr dom;
+ /* inactive domain config in case of active persistent domain */
+ virDomainDefPtr persistDom;
virObjectPtr cookie;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f7c1d6f..7e0585d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15687,6 +15687,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
goto endjob;
+ if (virDomainObjIsActive(vm) && vm->persistent &&
+ !(def->persistDom = qemuDomainDefCopy(driver, vm->newDef,
+ VIR_DOMAIN_XML_SECURE |
+ VIR_DOMAIN_XML_MIGRATABLE)))
+ goto endjob;
+
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
@@ -16219,6 +16225,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainObjPrivatePtr priv;
int rc;
virDomainDefPtr config = NULL;
+ virDomainDefPtr persistConfig = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
bool was_stopped = false;
@@ -16226,6 +16233,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virCPUDefPtr origCPU = NULL;
unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
+ virDomainDefPtr newConfig = NULL;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
@@ -16332,6 +16340,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
+ if (snap->def->persistDom &&
+ !(persistConfig = virDomainDefCopy(snap->def->persistDom, caps,
+ driver->xmlopt, NULL, true)))
+ goto endjob;
+
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
switch ((virDomainState) snap->def->state) {
@@ -16440,8 +16453,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
* failed loadvm attempt? */
goto endjob;
}
- if (config) {
- virDomainObjAssignDef(vm, config, false, NULL);
+
+ /* Older versions do not save inactive config in metadata, instead
+ * they use active config for this purpose, so keep this behaviour
+ * for backward compat.
+ */
+ if (persistConfig)
+ VIR_STEAL_PTR(newConfig, persistConfig);
+ else
+ VIR_STEAL_PTR(newConfig, config);
+
+ if (newConfig) {
+ virDomainObjAssignDef(vm, newConfig, false, NULL);
virCPUDefFree(priv->origCPU);
VIR_STEAL_PTR(priv->origCPU, origCPU);
}
@@ -16449,8 +16472,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 2, 3 */
load:
was_stopped = true;
- if (config)
+ if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
+ config = NULL;
+ }
/* No cookie means libvirt which saved the domain was too old to
* mess up the CPU definitions.
@@ -16471,6 +16496,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
detail);
if (rc < 0)
goto endjob;
+
+ if (persistConfig) {
+ virDomainObjAssignDef(vm, persistConfig, false, NULL);
+ VIR_STEAL_PTR(newConfig, persistConfig);
+ }
}
/* Touch up domain state. */
@@ -16535,8 +16565,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainRemoveInactive(driver, vm);
goto endjob;
}
- if (config)
+ if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
+ VIR_STEAL_PTR(newConfig, config);
+ }
if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
@@ -16600,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
} else if (snap) {
snap->def->current = false;
}
- if (ret == 0 && config && vm->persistent &&
+ if (ret == 0 && newConfig && vm->persistent &&
!(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
vm->newDef ? vm->newDef : vm->def))) {
detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT;
@@ -16616,6 +16648,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virObjectUnref(cfg);
virNWFilterUnlockFilterUpdates();
virCPUDefFree(origCPU);
+ virDomainDefFree(config);
+ virDomainDefFree(persistConfig);
return ret;
}
--
1.8.3.1
2:31 p.m.
New subject: [libvirt] [PATCH 3/9] qemu: snapshot: save/restore inactive persistent config
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
In case of active persistent domain snapshot metadata is
not complete. We save only active configuration and on
restore use it both for active and inactive configuration.
Let's fix it and save and restore both in this case.
In case of active transient or inactive domain we have
only one config and thus everything is good.
By the way this patch fixes config memleak on error paths.
use @config to make it clearer...
I see how/why you combined things and although extracting out the
memleak is a bit difficult, I think it should be done.
Perhaps in the cleanup code to *SaveConfig, you just set @config = NULL
"temporarily" since we know/assume virDomainObjAssignDef was successful
and we wouldn't want the subsequently added virDomainDefFree(config) to
free it on those paths, but we would want it to be free'd on error
paths. Then by using @newConfig in this patch, it's even more obvious
and of course the temporary config = NULL would be removed... Hope this
makes sense.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 1 +
src/conf/snapshot_conf.h | 2 ++
src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++++++++++++-----
3 files changed, 42 insertions(+), 5 deletions(-)
Not my area of expertise, but rather than have it sit unreviewed...
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 5a511b4..2e4a0b9 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
virDomainSnapshotDiskDefClear(&def->disks[i]);
VIR_FREE(def->disks);
virDomainDefFree(def->dom);
+ virDomainDefFree(def->persistDom);
virObjectUnref(def->cookie);
VIR_FREE(def);
}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 20a42bd..3da204a 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -75,6 +75,8 @@ struct _virDomainSnapshotDef {
virDomainSnapshotDiskDef *disks;
virDomainDefPtr dom;
+ /* inactive domain config in case of active persistent domain */
+ virDomainDefPtr persistDom;
virObjectPtr cookie;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f7c1d6f..7e0585d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15687,6 +15687,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
goto endjob;
+ if (virDomainObjIsActive(vm) && vm->persistent &&
As it's easy to get lost or forget... IIRC vm->persistent means that we
have used the Define functions to provide a "config" at some point in
time. Then eventually when/if we modify the "active" object to make a
configuration specific change, then the @newDef gets populated with the
config change(s); otherwise, the changes are made to the "active" def
and eventually/possibly saved. All the magic is hidden away in
accessors that I don't think about all that often any more.
So, should the vm->persistent be vm->newDef instead? Either way if the
following call is made and vm->newDef == NULL, then eventually in
virDomainDefFormatInternal we would core when @def was NULL.
+ !(def->persistDom = qemuDomainDefCopy(driver,
vm->newDef,
+ VIR_DOMAIN_XML_SECURE |
+ VIR_DOMAIN_XML_MIGRATABLE)))
Do we need VIR_DOMAIN_XML_INACTIVE too since this is the "next"
persistent config def? A variant for parse is used later in patch5.
+ goto endjob;
+
Since we are doing this as "best chance" and aren't requiring it, what
are your (and perhaps others) thoughts related to making this really
optional. As in if the qemuDomainDefCopy fails, then we just throw a
VIR_INFO, clear the last error, and continue on?
I don't have a preference, but I'm just throwing it out there as an idea.
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
@@ -16219,6 +16225,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainObjPrivatePtr priv;
int rc;
virDomainDefPtr config = NULL;
+ virDomainDefPtr persistConfig = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
bool was_stopped = false;
@@ -16226,6 +16233,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virCPUDefPtr origCPU = NULL;
unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
+ virDomainDefPtr newConfig = NULL;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
@@ -16332,6 +16340,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
+ if (snap->def->persistDom &&
+ !(persistConfig = virDomainDefCopy(snap->def->persistDom, caps,
+ driver->xmlopt, NULL, true)))
+ goto endjob;
+
Based only on my previous comment, for this part I agree especially
since if we were able to save a config, then we'd want to be sure to
restore it.
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
switch ((virDomainState) snap->def->state) {
@@ -16440,8 +16453,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
* failed loadvm attempt? */
goto endjob;
}
- if (config) {
- virDomainObjAssignDef(vm, config, false, NULL);
+
+ /* Older versions do not save inactive config in metadata, instead
+ * they use active config for this purpose, so keep this behaviour
+ * for backward compat.
+ */
+ if (persistConfig)
+ VIR_STEAL_PTR(newConfig, persistConfig);
+ else
+ VIR_STEAL_PTR(newConfig, config);
+
+ if (newConfig) {
+ virDomainObjAssignDef(vm, newConfig, false, NULL);
virCPUDefFree(priv->origCPU);
VIR_STEAL_PTR(priv->origCPU, origCPU);
}
@@ -16449,8 +16472,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Transitions 2, 3 */
load:
was_stopped = true;
- if (config)
+ if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
+ config = NULL;
This "makes sense" except for the case where @persistConfig == NULL,
then we wouldn't virDomainSaveConfig below as we currently do. So maybe
this should be:
if (!persistConfig)
VIR_STEAL_PTR(newConfig, config);
else
config = NULL;
That way we won't virDomainDefFree @config below (which is what you're
trying to avoid), but we will save @config as we did before unless of
course @persistConfig is going to be used.
Make sense?
+ }
/* No cookie means libvirt which saved the domain was too old to
* mess up the CPU definitions.
@@ -16471,6 +16496,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
detail);
if (rc < 0)
goto endjob;
+
+ if (persistConfig) {
Perhaps we should point out:
/* This will save the @persistConfig in the vm->newDef where it
* was originally copied from. */
John
+ virDomainObjAssignDef(vm, persistConfig, false,
NULL);
+ VIR_STEAL_PTR(newConfig, persistConfig);
+ }
}
/* Touch up domain state. */
@@ -16535,8 +16565,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainRemoveInactive(driver, vm);
goto endjob;
}
- if (config)
+ if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
+ VIR_STEAL_PTR(newConfig, config);
+ }
if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
@@ -16600,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
} else if (snap) {
snap->def->current = false;
}
- if (ret == 0 && config && vm->persistent &&
+ if (ret == 0 && newConfig && vm->persistent &&
!(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
vm->newDef ? vm->newDef : vm->def))) {
detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT;
@@ -16616,6 +16648,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virObjectUnref(cfg);
virNWFilterUnlockFilterUpdates();
virCPUDefFree(origCPU);
+ virDomainDefFree(config);
+ virDomainDefFree(persistConfig);
return ret;
}
2:23 a.m.
New subject: [libvirt] [PATCH 3/9] qemu: snapshot: save/restore inactive persistent config
On 20.12.2018 23:31, John Ferlan wrote:
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
> In case of active persistent domain snapshot metadata is
> not complete. We save only active configuration and on
> restore use it both for active and inactive configuration.
> Let's fix it and save and restore both in this case.
>
> In case of active transient or inactive domain we have
> only one config and thus everything is good.
>
> By the way this patch fixes config memleak on error paths.
use @config to make it clearer...
I see how/why you combined things and although extracting out the
memleak is a bit difficult, I think it should be done.
Perhaps in the cleanup code to *SaveConfig, you just set @config = NULL
"temporarily" since we know/assume virDomainObjAssignDef was successful
and we wouldn't want the subsequently added virDomainDefFree(config) to
free it on those paths, but we would want it to be free'd on error
paths. Then by using @newConfig in this patch, it's even more obvious
and of course the temporary config = NULL would be removed... Hope this
makes sense.
Ok. I thought of a distinct patch too, but having in mind that adding/removing
code in subsequent patches is not preferred I decided to just note this moment
in the message.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/conf/snapshot_conf.c | 1 +
> src/conf/snapshot_conf.h | 2 ++
> src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 42 insertions(+), 5 deletions(-)
>
Not my area of expertise, but rather than have it sit unreviewed...
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 5a511b4..2e4a0b9 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
> virDomainSnapshotDiskDefClear(&def->disks[i]);
> VIR_FREE(def->disks);
> virDomainDefFree(def->dom);
> + virDomainDefFree(def->persistDom);
> virObjectUnref(def->cookie);
> VIR_FREE(def);
> }
> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
> index 20a42bd..3da204a 100644
> --- a/src/conf/snapshot_conf.h
> +++ b/src/conf/snapshot_conf.h
> @@ -75,6 +75,8 @@ struct _virDomainSnapshotDef {
> virDomainSnapshotDiskDef *disks;
>
> virDomainDefPtr dom;
> + /* inactive domain config in case of active persistent domain */
> + virDomainDefPtr persistDom;
>
> virObjectPtr cookie;
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f7c1d6f..7e0585d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15687,6 +15687,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
> goto endjob;
>
> + if (virDomainObjIsActive(vm) && vm->persistent &&
As it's easy to get lost or forget... IIRC vm->persistent means that w> have
used the Define functions to provide a "config" at some point in
time. Then eventually when/if we modify the "active" object
to make a
configuration specific change, then the @newDef gets populated with the
config change(s); otherwise, the changes are made to the "active" def
and eventually/possibly saved. All the magic is hidden away in
accessors that I don't think about all that often any more.
If by otherwise you mean "if we make changes to active config" - then
such changes are saved to status files but they can't be written
directly to config file.
So, should the vm->persistent be vm->newDef instead? Either way if the
following call is made and vm->newDef == NULL, then eventually in
virDomainDefFormatInternal we would core when @def was NULL.
Well there is at least one case when vm->newDef != NULL but domain is not persistent
- if we undefine domain vm->newDef is not released. This can be fixed but
I'm not sure there are no other cases. So checking vm->persisent is more reliable.
I think this is currenly invariant that if virDomainObjIsActive(vm) &&
vm->persistent
then vm->newDef != NULL but we can add explicit check.
The best option I think is to fix all cases when vm->newDef != NULL for non persistent
active domain and replace check as you suggested. What do you think?
> + !(def->persistDom = qemuDomainDefCopy(driver, vm->newDef,
> + VIR_DOMAIN_XML_SECURE |
> + VIR_DOMAIN_XML_MIGRATABLE)))
Do we need VIR_DOMAIN_XML_INACTIVE too since this is the "next"
persistent config def? A variant for parse is used later in patch5.
This is one of the hardest questions - what combinations of flags one should
use for one's purpuses :) I'm not kidding, there are no good definitions of
VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_DEF_FORMAT_INACTIVE. So I use same
combination migration uses to pass persistent config to destination -
check qemuMigrationSrcRun.
Hmm, virDomainDefFormatInternal has next hunk at the beginning:
if (def->id == -1)
flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
and def->id == -1 for inactive config (@id is set to -1 on parsing
inactive configs and on domain start @def is copied to @newDef before
allocating @id - check qemuProcessInit). So we have this flag anyway :)
Also even the above fact does not have the case AFAIU having this flag for inactive
config
does not have much meaning - random hunks of code that I checked filter
dumping info that gained by config on domain start. However I run across a
kind of exception too - @id is not dumped at all if VIR_DOMAIN_DEF_FORMAT_INACTIVE
is set however this is not make much difference.
> + goto endjob;
> +
Since we are doing this as "best chance" and aren't requiring it, what
are your (and perhaps others) thoughts related to making this really
optional. As in if the qemuDomainDefCopy fails, then we just throw a
VIR_INFO, clear the last error, and continue on?
I don't have a preference, but I'm just throwing it out there as an idea.
I should say I'm not looking at this step as "best chance" - if we can
not revert exact state I'd prefer to fail here.
> if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
> align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
> align_match = false;
> @@ -16219,6 +16225,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> qemuDomainObjPrivatePtr priv;
> int rc;
> virDomainDefPtr config = NULL;
> + virDomainDefPtr persistConfig = NULL;
> virQEMUDriverConfigPtr cfg = NULL;
> virCapsPtr caps = NULL;
> bool was_stopped = false;
> @@ -16226,6 +16233,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> virCPUDefPtr origCPU = NULL;
> unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
> qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
> + virDomainDefPtr newConfig = NULL;
>
> virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
> VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
> @@ -16332,6 +16340,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> goto endjob;
> }
>
> + if (snap->def->persistDom &&
> + !(persistConfig = virDomainDefCopy(snap->def->persistDom, caps,
> + driver->xmlopt, NULL, true)))
> + goto endjob;
> +
Based only on my previous comment, for this part I agree especially
since if we were able to save a config, then we'd want to be sure to
restore it.
> cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
>
> switch ((virDomainState) snap->def->state) {
> @@ -16440,8 +16453,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> * failed loadvm attempt? */
> goto endjob;
> }
> - if (config) {
> - virDomainObjAssignDef(vm, config, false, NULL);
> +
> + /* Older versions do not save inactive config in metadata, instead
> + * they use active config for this purpose, so keep this behaviour
> + * for backward compat.
> + */
> + if (persistConfig)
> + VIR_STEAL_PTR(newConfig, persistConfig);
> + else
> + VIR_STEAL_PTR(newConfig, config);
> +
> + if (newConfig) {
> + virDomainObjAssignDef(vm, newConfig, false, NULL);
> virCPUDefFree(priv->origCPU);
> VIR_STEAL_PTR(priv->origCPU, origCPU);
> }
> @@ -16449,8 +16472,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> /* Transitions 2, 3 */
> load:
> was_stopped = true;
> - if (config)
> + if (config) {
> virDomainObjAssignDef(vm, config, false, NULL);
> + config = NULL;
This "makes sense" except for the case where @persistConfig == NULL,
then we wouldn't virDomainSaveConfig below as we currently do. So maybe
this should be:
if (!persistConfig)
VIR_STEAL_PTR(newConfig, config);
else
config = NULL;
That way we won't virDomainDefFree @config below (which is what you're
trying to avoid), but we will save @config as we did before unless of
course @persistConfig is going to be used.
Make sense?
Good catch! I think I'll change to:
if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
VIR_STEAL_PTR(newConfig, config);
}
because if we have persistConfig but fail to assing it - means
we are on error path and we won't save newConfig anyway.
> + }
>
> /* No cookie means libvirt which saved the domain was too old to
> * mess up the CPU definitions.
> @@ -16471,6 +16496,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> detail);
> if (rc < 0)
> goto endjob;
> +
> + if (persistConfig) {
Perhaps we should point out:
/* This will save the @persistConfig in the vm->newDef where it
* was originally copied from. */
John
> + virDomainObjAssignDef(vm, persistConfig, false, NULL);
> + VIR_STEAL_PTR(newConfig, persistConfig);
> + }
> }
This makes me wonder if virDomainObjAssignDef is too overcomplicated
if we need such comments.
Nikolay
>
> /* Touch up domain state. */
> @@ -16535,8 +16565,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> qemuDomainRemoveInactive(driver, vm);
> goto endjob;
> }
> - if (config)
> + if (config) {
> virDomainObjAssignDef(vm, config, false, NULL);
> + VIR_STEAL_PTR(newConfig, config);
> + }
>
> if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
> VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
> @@ -16600,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> } else if (snap) {
> snap->def->current = false;
> }
> - if (ret == 0 && config && vm->persistent &&
> + if (ret == 0 && newConfig && vm->persistent &&
> !(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
> vm->newDef ? vm->newDef : vm->def))) {
> detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT;
> @@ -16616,6 +16648,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> virObjectUnref(cfg);
> virNWFilterUnlockFilterUpdates();
> virCPUDefFree(origCPU);
> + virDomainDefFree(config);
> + virDomainDefFree(persistConfig);
>
> return ret;
> }
>
8:03 a.m.
New subject: [libvirt] [PATCH 3/9] qemu: snapshot: save/restore inactive persistent config
On 12/21/18 3:23 AM, Nikolay Shirokovskiy wrote:
On 20.12.2018 23:31, John Ferlan wrote:
>
>
> On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
>> In case of active persistent domain snapshot metadata is
>> not complete. We save only active configuration and on
>> restore use it both for active and inactive configuration.
>> Let's fix it and save and restore both in this case.
>>
>> In case of active transient or inactive domain we have
>> only one config and thus everything is good.
>>
>> By the way this patch fixes config memleak on error paths.
>
> use @config to make it clearer...
>
> I see how/why you combined things and although extracting out the
> memleak is a bit difficult, I think it should be done.
>
> Perhaps in the cleanup code to *SaveConfig, you just set @config = NULL
> "temporarily" since we know/assume virDomainObjAssignDef was successful
> and we wouldn't want the subsequently added virDomainDefFree(config) to
> free it on those paths, but we would want it to be free'd on error
> paths. Then by using @newConfig in this patch, it's even more obvious
> and of course the temporary config = NULL would be removed... Hope this
> makes sense.
Ok. I thought of a distinct patch too, but having in mind that adding/removing
code in subsequent patches is not preferred I decided to just note this moment
in the message.
I'm mostly 50/50 on this - leaning towards separation mainly because I
think it is possible, but also because when you mention adding a new
feature and fixing an existing memory leak in the commit message that
generally results in someone asking for separation. Certainly makes it
easier to bisect to a commit and not have to wonder did the new
functionality or the bugfix cause some issue...
>
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
>> ---
>> src/conf/snapshot_conf.c | 1 +
>> src/conf/snapshot_conf.h | 2 ++
>> src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++++++++++++-----
>> 3 files changed, 42 insertions(+), 5 deletions(-)
>>
>
> Not my area of expertise, but rather than have it sit unreviewed...
>
>
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 5a511b4..2e4a0b9 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
>> virDomainSnapshotDiskDefClear(&def->disks[i]);
>> VIR_FREE(def->disks);
>> virDomainDefFree(def->dom);
>> + virDomainDefFree(def->persistDom);
>> virObjectUnref(def->cookie);
>> VIR_FREE(def);
>> }
>> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
>> index 20a42bd..3da204a 100644
>> --- a/src/conf/snapshot_conf.h
>> +++ b/src/conf/snapshot_conf.h
>> @@ -75,6 +75,8 @@ struct _virDomainSnapshotDef {
>> virDomainSnapshotDiskDef *disks;
>>
>> virDomainDefPtr dom;
>> + /* inactive domain config in case of active persistent domain */
>> + virDomainDefPtr persistDom;
>>
>> virObjectPtr cookie;
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index f7c1d6f..7e0585d 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -15687,6 +15687,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>>
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
>> goto endjob;
>>
>> + if (virDomainObjIsActive(vm) && vm->persistent &&
>
> As it's easy to get lost or forget... IIRC vm->persistent means that w>
have used the Define functions to provide a "config" at some point in
> time. Then eventually when/if we modify the "active" object to make a
> configuration specific change, then the @newDef gets populated with the
> config change(s); otherwise, the changes are made to the "active" def
> and eventually/possibly saved. All the magic is hidden away in
> accessors that I don't think about all that often any more.
If by otherwise you mean "if we make changes to active config" - then
such changes are saved to status files but they can't be written
directly to config file.
Like I said - some of the magic and details are lost in accessors. Yes,
the active is essentially saved to status... The whole def/newDef fetch,
store, and manipulation gets hidden behind those *Domain.*Persistent*
type helpers. The @newDef nomenclature has always struck me as a bit
odd, but using a longer more descriptive name may not help me either.
>
> So, should the vm->persistent be vm->newDef instead? Either way if the
> following call is made and vm->newDef == NULL, then eventually in
> virDomainDefFormatInternal we would core when @def was NULL.
Well there is at least one case when vm->newDef != NULL but domain is not persistent
- if we undefine domain vm->newDef is not released. This can be fixed but
I'm not sure there are no other cases. So checking vm->persisent is more reliable.
So we have an active persistent domain with a newDef that eventually is
made non persistent... I'm sure there's a reason for it...
I think this is currenly invariant that if virDomainObjIsActive(vm) &&
vm->persistent
then vm->newDef != NULL but we can add explicit check.
The best option I think is to fix all cases when vm->newDef != NULL for non
persistent
active domain and replace check as you suggested. What do you think?
I think my main concern would be vm->persistent == true and vm->newDef
== NULL.
>
>
>> + !(def->persistDom = qemuDomainDefCopy(driver, vm->newDef,
>> + VIR_DOMAIN_XML_SECURE |
>> + VIR_DOMAIN_XML_MIGRATABLE)))
>
> Do we need VIR_DOMAIN_XML_INACTIVE too since this is the "next"
> persistent config def? A variant for parse is used later in patch5.
This is one of the hardest questions - what combinations of flags one should
use for one's purpuses :) I'm not kidding, there are no good definitions of
110% agree.
VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_DEF_FORMAT_INACTIVE. So I use
same
combination migration uses to pass persistent config to destination -
check qemuMigrationSrcRun.
and obviously I asked because I find the design of the flags is
confusing and what each means with respect to what post parse processing
or validation occurs - I just cannot keep it in memory.
Hmm, virDomainDefFormatInternal has next hunk at the beginning:
if (def->id == -1)
flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
and def->id == -1 for inactive config (@id is set to -1 on parsing
inactive configs and on domain start @def is copied to @newDef before
allocating @id - check qemuProcessInit). So we have this flag anyway :)
<sigh>... commit f24e67d24
Also even the above fact does not have the case AFAIU having this flag for inactive
config
does not have much meaning - random hunks of code that I checked filter
dumping info that gained by config on domain start. However I run across a
kind of exception too - @id is not dumped at all if VIR_DOMAIN_DEF_FORMAT_INACTIVE
is set however this is not make much difference.
The case you're chasing though would have an id != -1 since it's an
active guest, but we're saving the newDef/next config. Perhaps similar
to how various qemu_driver commands can manipulate both active and
config (attach, detach, change, etc). It's a "hypervisor dependent"
action as to what happens.
>
>> + goto endjob;
>> +
>
> Since we are doing this as "best chance" and aren't requiring it, what
> are your (and perhaps others) thoughts related to making this really
> optional. As in if the qemuDomainDefCopy fails, then we just throw a
> VIR_INFO, clear the last error, and continue on?
>
> I don't have a preference, but I'm just throwing it out there as an idea.
I should say I'm not looking at this step as "best chance" - if we can
not revert exact state I'd prefer to fail here.
>
>> if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
>> align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>> align_match = false;
>> @@ -16219,6 +16225,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> qemuDomainObjPrivatePtr priv;
>> int rc;
>> virDomainDefPtr config = NULL;
>> + virDomainDefPtr persistConfig = NULL;
>> virQEMUDriverConfigPtr cfg = NULL;
>> virCapsPtr caps = NULL;
>> bool was_stopped = false;
>> @@ -16226,6 +16233,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> virCPUDefPtr origCPU = NULL;
>> unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
>> qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
>> + virDomainDefPtr newConfig = NULL;
>>
>> virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>> VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
>> @@ -16332,6 +16340,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> goto endjob;
>> }
>>
>> + if (snap->def->persistDom &&
>> + !(persistConfig = virDomainDefCopy(snap->def->persistDom, caps,
>> + driver->xmlopt, NULL, true)))
>> + goto endjob;
>> +
>
> Based only on my previous comment, for this part I agree especially
> since if we were able to save a config, then we'd want to be sure to
> restore it.
>
>> cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
>>
>> switch ((virDomainState) snap->def->state) {
>> @@ -16440,8 +16453,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> * failed loadvm attempt? */
>> goto endjob;
>> }
>> - if (config) {
>> - virDomainObjAssignDef(vm, config, false, NULL);
>> +
>> + /* Older versions do not save inactive config in metadata, instead
>> + * they use active config for this purpose, so keep this behaviour
>> + * for backward compat.
>> + */
>> + if (persistConfig)
>> + VIR_STEAL_PTR(newConfig, persistConfig);
>> + else
>> + VIR_STEAL_PTR(newConfig, config);
>> +
>> + if (newConfig) {
>> + virDomainObjAssignDef(vm, newConfig, false, NULL);
>> virCPUDefFree(priv->origCPU);
>> VIR_STEAL_PTR(priv->origCPU, origCPU);
>> }
>> @@ -16449,8 +16472,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> /* Transitions 2, 3 */
>> load:
>> was_stopped = true;
>> - if (config)
>> + if (config) {
>> virDomainObjAssignDef(vm, config, false, NULL);
>> + config = NULL;
>
> This "makes sense" except for the case where @persistConfig == NULL,
> then we wouldn't virDomainSaveConfig below as we currently do. So maybe
> this should be:
>
> if (!persistConfig)
> VIR_STEAL_PTR(newConfig, config);
> else
> config = NULL;
>
> That way we won't virDomainDefFree @config below (which is what you're
> trying to avoid), but we will save @config as we did before unless of
> course @persistConfig is going to be used.
>
> Make sense?
Good catch! I think I'll change to:
if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
VIR_STEAL_PTR(newConfig, config);
}
because if we have persistConfig but fail to assing it - means
we are on error path and we won't save newConfig anyway.
OK - I was going explicit with the alternative to the code below for
persistConfig/newConfig.
>
>> + }
>>
>> /* No cookie means libvirt which saved the domain was too old to
>> * mess up the CPU definitions.
>> @@ -16471,6 +16496,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> detail);
>> if (rc < 0)
>> goto endjob;
>> +
>> + if (persistConfig) {
>
> Perhaps we should point out:
>
> /* This will save the @persistConfig in the vm->newDef where it
> * was originally copied from. */
>
> John
>
>> + virDomainObjAssignDef(vm, persistConfig, false, NULL);
>> + VIR_STEAL_PTR(newConfig, persistConfig);
>> + }
>> }
This makes me wonder if virDomainObjAssignDef is too overcomplicated
if we need such comments.
Nah, no way that's over complicated <eyeroll>. Touching it though means
you get to own making it better... job security and all that.
John
Nikolay
>>
>> /* Touch up domain state. */
>> @@ -16535,8 +16565,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> qemuDomainRemoveInactive(driver, vm);
>> goto endjob;
>> }
>> - if (config)
>> + if (config) {
>> virDomainObjAssignDef(vm, config, false, NULL);
>> + VIR_STEAL_PTR(newConfig, config);
>> + }
>>
>> if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>> VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
>> @@ -16600,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> } else if (snap) {
>> snap->def->current = false;
>> }
>> - if (ret == 0 && config && vm->persistent &&
>> + if (ret == 0 && newConfig && vm->persistent &&
>> !(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
>> vm->newDef ? vm->newDef :
vm->def))) {
>> detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT;
>> @@ -16616,6 +16648,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
>> virObjectUnref(cfg);
>> virNWFilterUnlockFilterUpdates();
>> virCPUDefFree(origCPU);
>> + virDomainDefFree(config);
>> + virDomainDefFree(persistConfig);
>>
>> return ret;
>> }
>>
2:03 a.m.
New subject: [libvirt] [PATCH 4/9] schema: snapshot: add persistent domain config
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
docs/schemas/domainsnapshot.rng | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng
index 2680887..405e614 100644
--- a/docs/schemas/domainsnapshot.rng
+++ b/docs/schemas/domainsnapshot.rng
@@ -84,6 +84,16 @@
</choice>
</optional>
<optional>
+ <element name='persistent'>
+ <!-- Nested grammar ensures that any of our overrides of
+ storagecommon/domaincommon defines do not conflict
+ with any domain.rng overrides. -->
+ <grammar>
+ <include href='domain.rng'/>
+ </grammar>
+ </element>
+ </optional>
+ <optional>
<element name='parent'>
<element name='name'>
<text/>
--
1.8.3.1
2:32 p.m.
New subject: [libvirt] [PATCH 4/9] schema: snapshot: add persistent domain config
Maybe a few words here about the option being used to hold a persistent
config that could be different than the snapshot's running domain def.
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
docs/schemas/domainsnapshot.rng | 10 ++++++++++
1 file changed, 10 insertions(+)
Seems reasonable - although I wonder if this should be combined with the
next two patches as well... That's not a big deal though...
I actually prefer "persistent" over "newDef" or a/k/a
"nextPersistentDef".
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:03 a.m.
New subject: [libvirt] [PATCH 5/9] conf: snapshot: dump/parse persistent domain config
Persistent in this case is a persistent config of active
persistent domain. We need it for metadata to be complete
as explained in previous patch.
Config is saved like this:
<domainsnapshot>
<persistent>
<domain>
...
</domain>
</persistent>
</domainsnapshot>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 26 +++++++
.../active_inactive_domain.xml | 85 ++++++++++++++++++++++
tests/domainsnapshotxml2xmltest.c | 1 +
3 files changed, 112 insertions(+)
create mode 100644 tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 2e4a0b9..34fcf64 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -283,6 +283,20 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
} else {
VIR_WARN("parsing older snapshot that lacks domain");
}
+
+ if (def->state == VIR_DOMAIN_RUNNING ||
+ def->state == VIR_DOMAIN_PAUSED) {
+ int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+ VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
+ xmlNodePtr domainNode;
+
+ if ((domainNode = virXPathNode("./persistent/domain", ctxt))
&&
+ !(def->persistDom = virDomainDefParseNode(ctxt->node->doc,
+ domainNode, caps,
+ xmlopt, NULL,
+ domainflags)))
+ goto cleanup;
+ }
} else {
def->creationTime = tv.tv_sec;
}
@@ -753,6 +767,18 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
virBufferAddLit(&buf, "</domain>\n");
}
+ if (def->persistDom) {
+ virBufferAddLit(&buf, "<persistent>\n");
+ virBufferAdjustIndent(&buf, 2);
+
+ if (virDomainDefFormatInternal(def->persistDom,
+ caps, flags, &buf, xmlopt) < 0)
+ goto error;
+
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</persistent>\n");
+ }
+
if (virSaveCookieFormatBuf(&buf, def->cookie,
virDomainXMLOptionGetSaveCookie(xmlopt)) < 0)
goto error;
diff --git a/tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
b/tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
new file mode 100644
index 0000000..74025a3
--- /dev/null
+++ b/tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
@@ -0,0 +1,85 @@
+<domainsnapshot>
+ <name>my snap name</name>
+ <description>!@#$%^</description>
+ <state>running</state>
+ <parent>
+ <name>earlier_snap</name>
+ </parent>
+ <creationTime>1272917631</creationTime>
+ <memory snapshot='internal'/>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+ </domain>
+ <persistent>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'
cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0'
model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+ </domain>
+ </persistent>
+ <active>1</active>
+</domainsnapshot>
diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c
index 2a07fe0..e51373d 100644
--- a/tests/domainsnapshotxml2xmltest.c
+++ b/tests/domainsnapshotxml2xmltest.c
@@ -204,6 +204,7 @@ mymain(void)
DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8",
false);
DO_TEST_OUT("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809",
false);
DO_TEST_OUT("external_vm_redefine",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", false);
+ DO_TEST_OUT("active_inactive_domain", NULL, true);
DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8",
false, false);
DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8",
false, false);
--
1.8.3.1
2:32 p.m.
New subject: [libvirt] [PATCH 5/9] conf: snapshot: dump/parse persistent domain config
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Persistent in this case is a persistent config of active
of the active
persistent domain. We need it for metadata to be complete
as explained in previous patch.
Hence why I think the two should be combined.
Config is saved like this:
<domainsnapshot>
<persistent>
<domain>
...
</domain>
</persistent>
</domainsnapshot>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 26 +++++++
.../active_inactive_domain.xml | 85 ++++++++++++++++++++++
tests/domainsnapshotxml2xmltest.c | 1 +
3 files changed, 112 insertions(+)
create mode 100644 tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
Seems reasonable to me...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:03 a.m.
New subject: [libvirt] [PATCH 6/9] docs: add persistent config to snapshot xml description
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
docs/formatsnapshot.html.in | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in
index fbbecfd..1357f53 100644
--- a/docs/formatsnapshot.html.in
+++ b/docs/formatsnapshot.html.in
@@ -235,6 +235,13 @@
at the time of the snapshot (<span class="since">since
0.9.5</span>). Readonly.
</dd>
+ <dt><code>persistent/domain</code></dt>
+ <dd>Inactive domain configuration for active persistent domain.
+ Such a domain have 2 distinct configs and here inactive is
+ stored. It is different from <code>domain</code> which more
+ presisely keeps "inactive portion" of active config.
+ (<span class="since">since 5.0.0</span>).
+ </dd>
<dt><code>cookie</code></dt>
<dd>Save image cookie containing additional data libvirt may need to
properly restore a domain from an active snapshot when such data
--
1.8.3.1
2:32 p.m.
New subject: [libvirt] [PATCH 6/9] docs: add persistent config to snapshot xml description
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
docs/formatsnapshot.html.in | 7 +++++++
1 file changed, 7 insertions(+)
This probably should be merged with patch5 - nice to separate for review
though. Although, like patch4 it's not that important to me.
diff --git a/docs/formatsnapshot.html.in
b/docs/formatsnapshot.html.in
index fbbecfd..1357f53 100644
--- a/docs/formatsnapshot.html.in
+++ b/docs/formatsnapshot.html.in
@@ -235,6 +235,13 @@
at the time of the snapshot (<span class="since">since
0.9.5</span>). Readonly.
</dd>
+ <dt><code>persistent/domain</code></dt>
+ <dd>Inactive domain configuration for active persistent domain.
s/for/from an/
+ Such a domain have 2 distinct configs and here inactive is
Tough read grammar wise.
+ stored. It is different from <code>domain</code>
which more
+ presisely keeps "inactive portion" of active config.
precisely
+ (<span class="since">since
5.0.0</span>).
+ </dd>
Hmmm... IIRC... isn't newDef the "next config"? Scour around for
Domain*Persistent and see what I mean. I think you have this description
backwards - hey I could be wrong, too...
An active domain for which configuration specific changes have been made
will store both the "active" and "next config" in the domain object
and
the <persistent> element will signify that config; whereas, the <domain>
element signifies the active domain configuration.
There is nothing different in the previous patch between the "<domain
... </domain>" and the "<persistent> ... </persistent>";
however, I
would believe that if something changed as "config only" then it
would/should show up in that persistDom.
John
<dt><code>cookie</code></dt>
<dd>Save image cookie containing additional data libvirt may need to
properly restore a domain from an active snapshot when such data
2:50 a.m.
New subject: [libvirt] [PATCH 6/9] docs: add persistent config to snapshot xml description
On 20.12.2018 23:32, John Ferlan wrote:
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> docs/formatsnapshot.html.in | 7 +++++++
> 1 file changed, 7 insertions(+)
>
This probably should be merged with patch5 - nice to separate for review
though. Although, like patch4 it's not that important to me.
> diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in
> index fbbecfd..1357f53 100644
> --- a/docs/formatsnapshot.html.in
> +++ b/docs/formatsnapshot.html.in
> @@ -235,6 +235,13 @@
> at the time of the snapshot (<span class="since">since
> 0.9.5</span>). Readonly.
> </dd>
> + <dt><code>persistent/domain</code></dt>
> + <dd>Inactive domain configuration for active persistent domain.
s/for/from an/
> + Such a domain have 2 distinct configs and here inactive is
Tough read grammar wise.
> + stored. It is different from <code>domain</code> which more
> + presisely keeps "inactive portion" of active config.
precisely
> + (<span class="since">since 5.0.0</span>).
> + </dd>
Hmmm... IIRC... isn't newDef the "next config"? Scour around for
Domain*Persistent and see what I mean. I think you have this description
backwards - hey I could be wrong, too...
An active domain for which configuration specific changes have been made
will store both the "active" and "next config" in the domain object
and
the <persistent> element will signify that config; whereas, the <domain>
element signifies the active domain configuration.
Naming is known to be hard. For active domain you can think of this
configs as active/persistent or active/next one(new one). I think
@newDef name is choosen because @persistentDef would be misleading -
inactive domains store its persistent domain in @def and not @newDef
(however I would prefer the opposite - inactive domains storing its
definition in @persistentDef and having @def = NULL).
If done from scratch I would suggest to use:
- <domain> for inactive/transient domains
- <persistent><domain> and <transient><domain> for active
persistent domains
or even only last option for all cases. But now for backward compat we
need some name. Well I thought explaining next/new def is more complicated
then stating that persistent is only defined for active persistent domains
and that is keeps the persistent config.
Nikolay
There is nothing different in the previous patch between the "<domain
... </domain>" and the "<persistent> ... </persistent>";
however, I
would believe that if something changed as "config only" then it
would/should show up in that persistDom.
John
> <dt><code>cookie</code></dt>
> <dd>Save image cookie containing additional data libvirt may need to
> properly restore a domain from an active snapshot when such data
>
2:03 a.m.
New subject: [libvirt] [PATCH 7/9] conf: snapshot: check domain name on redefine
Renaming domain which has snapshots is prohibited. Also reverting
to ABI compatible active domain with a different name can have
issues later I guess. So let's prohibit changing domain name on snapshot
metadata redefine as well.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 34fcf64..fa1cd6b 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
goto cleanup;
}
+ if (def->dom &&
+ STRNEQ(def->dom->name, domain->name)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("definition for snapshot %s must use name %s"),
+ def->name, domain->name);
+ goto cleanup;
+ }
+
other = virDomainSnapshotFindByName(vm->snapshots, def->name);
if (other) {
if ((other->def->state == VIR_DOMAIN_RUNNING ||
--
1.8.3.1
3:01 p.m.
New subject: [libvirt] [PATCH 7/9] conf: snapshot: check domain name on redefine
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Renaming domain which has snapshots is prohibited. Also reverting
to ABI compatible active domain with a different name can have
issues later I guess. So let's prohibit changing domain name on snapshot
metadata redefine as well.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 8 ++++++++
1 file changed, 8 insertions(+)
This is separable patch and I'm definitely not the expert here. I think
it should go before you make patch4 and beyond changes.
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 34fcf64..fa1cd6b 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
goto cleanup;
}
+ if (def->dom &&
+ STRNEQ(def->dom->name, domain->name)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("definition for snapshot %s must use name %s"),
+ def->name, domain->name);
+ goto cleanup;
+ }
+
You could have also done the "if (def->dom) { ... } " type change...
Not that it matters that much.
Although it seems reasonable to me, hopefully Peter will pipe in as
well. Reading the commit message makes me believe that this would not be
possible, but then again the UUID *and* name are used as keys to look up
the domain object so the names had better match. If they don't then we
could be in a heap of trouble. Not sure how it would happen though.
You have my R-By, but I'd certainly feel better with even more eyes and
thoughts on this.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
other = virDomainSnapshotFindByName(vm->snapshots,
def->name);
if (other) {
if ((other->def->state == VIR_DOMAIN_RUNNING ||
3 a.m.
New subject: [libvirt] [PATCH 7/9] conf: snapshot: check domain name on redefine
On 21.12.2018 00:01, John Ferlan wrote:
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
> Renaming domain which has snapshots is prohibited. Also reverting
> to ABI compatible active domain with a different name can have
> issues later I guess. So let's prohibit changing domain name on snapshot
> metadata redefine as well.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
> ---
> src/conf/snapshot_conf.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
This is separable patch and I'm definitely not the expert here. I think
it should go before you make patch4 and beyond changes.
> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index 34fcf64..fa1cd6b 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
> goto cleanup;
> }
>
> + if (def->dom &&
> + STRNEQ(def->dom->name, domain->name)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("definition for snapshot %s must use name %s"),
> + def->name, domain->name);
> + goto cleanup;
> + }
> +
You could have also done the "if (def->dom) { ... } " type change...
Not that it matters that much.
Although it seems reasonable to me, hopefully Peter will pipe in as
well. Reading the commit message makes me believe that this would not be
possible, but then again the UUID *and* name are used as keys to look up
the domain object so the names had better match. If they don't then we
could be in a heap of trouble. Not sure how it would happen though.
Yeah it is a bit defensive check as redefine is supposed to be feed by
xmls we get on dumping existing snapshot metadata so we should have
consistent names and uuids. I don't think anyone sane is going to change
uuid or name in xml so may be this check is excessive. UUID check along
is fine as this way we won't use snapshot metadata from different domain
by mistake. On the other hand it is good to have more checks as xmls
are editable in the end and only limited set of editings are valid.
Nikolay
You have my R-By, but I'd certainly feel better with even more eyes and
thoughts on this.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
> other = virDomainSnapshotFindByName(vm->snapshots, def->name);
> if (other) {
> if ((other->def->state == VIR_DOMAIN_RUNNING ||
>
2:03 a.m.
New subject: [libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine
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(+)
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;
+ }
+
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;
+ }
+
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;
+ }
goto cleanup;
}
}
--
1.8.3.1
3:15 p.m.
New subject: [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
3:08 a.m.
New subject: [libvirt] [PATCH 8/9] conf: snapshot: support persistent config on redefine
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(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;
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(a)redhat.com>
John
2:03 a.m.
New subject: [libvirt] [PATCH 9/9] news: add notice for persistent config in snapshot
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
docs/news.xml | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index a599ff4..79ebce9 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -52,6 +52,17 @@
<section title="Improvements">
<change>
<summary>
+ qemu: snapshot inactive config of active persistent domain too
+ </summary>
+ <description>
+ Previous libvirt versions do not store complete metadata for active
+ persistent domain snapshot. As a result on revert both active and
+ inactive configs are reverted to be the same. Now snapshot saves both
+ active and inactive configs.
+ </description>
+ </change>
+ <change>
+ <summary>
qemu: Add support for ARMv6l guests
</summary>
</change>
--
1.8.3.1
3:17 p.m.
New subject: [libvirt] [PATCH 9/9] news: add notice for persistent config in snapshot
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
docs/news.xml | 11 +++++++++++
1 file changed, 11 insertions(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
BTW: the explanation you give here matches my thoughts on this although
what I read and commented on in previous patches wasn't as clear at
least w/r/t how I read the patches...
2165
days inactive
2173
days old
23 comments
2 participants
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy