[libvirt PATCH 00/14] qemu snapshot revert cleanup and refactor
Pavel Hrdina (14): qemu_snapshot: revert: fix incorrect jump to cleanup qemu_snapshot: revert: drop unused variable qemu_snapshot: revert: use g_autoptr qemu_snapshot: revert: jump to endjob instead of calling qemuProcessEndJob qemu_snapshot: revert: remove cleanup label qemu_snapshot: revert: move validation to separate function qemu_snapshot: revert: move config prepare code to separate function qemu_snapshot: revert: emit event right after they are created qemu_snapshot: revert: refactor cleanup section qemu_snapshot: revert: move saving metadata to separate function qemu_snapshot: revert: save metadata within qemu process job qemu_snapshot: revert: move active snapshot revert to separate function qemu_snapshot: revert: rename qemuSnapshotRevertInactive qemu_snapshot: revert: move inactive snapshot to separate function src/qemu/qemu_snapshot.c | 554 ++++++++++++++++++++++----------------- 1 file changed, 320 insertions(+), 234 deletions(-) -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index edf5511e42..64b5838db8 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2087,7 +2087,7 @@ qemuSnapshotRevert(virDomainObj *vm, */ if (cookie && qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - goto cleanup; + goto endjob; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, -- 2.31.1
On a Thursday in 2021, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Fixes: 6a6f6b91e0e76480ea961f83135efcb4faf3284a
--- src/qemu/qemu_snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Commit <f33ce12e9cd9cab7e6022e91d3765c33d99bf777> dropped unused code but missed one variable. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 64b5838db8..413b31bae5 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1944,7 +1944,6 @@ qemuSnapshotRevert(virDomainObj *vm, virDomainDef *inactiveConfig = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainSaveCookie *cookie; - virCPUDef *origCPU = NULL; unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; bool defined = false; @@ -2234,7 +2233,6 @@ qemuSnapshotRevert(virDomainObj *vm, } virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, event2); - virCPUDefFree(origCPU); virDomainDefFree(config); virDomainDefFree(inactiveConfig); -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 413b31bae5..5ac1c99a68 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1940,8 +1940,8 @@ qemuSnapshotRevert(virDomainObj *vm, int detail; qemuDomainObjPrivate *priv = vm->privateData; int rc; - virDomainDef *config = NULL; - virDomainDef *inactiveConfig = NULL; + g_autoptr(virDomainDef) config = NULL; + g_autoptr(virDomainDef) inactiveConfig = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); qemuDomainSaveCookie *cookie; unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; @@ -2233,8 +2233,6 @@ qemuSnapshotRevert(virDomainObj *vm, } virObjectEventStateQueue(driver->domainEventState, event); virObjectEventStateQueue(driver->domainEventState, event2); - virDomainDefFree(config); - virDomainDefFree(inactiveConfig); return ret; } -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5ac1c99a68..8191733a6d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2149,8 +2149,7 @@ qemuSnapshotRevert(virDomainObj *vm, if (qemuSnapshotRevertInactive(driver, vm, snap) < 0) { qemuDomainRemoveInactive(driver, vm); - qemuProcessEndJob(driver, vm); - goto cleanup; + goto endjob; } if (inactiveConfig) { @@ -2173,8 +2172,7 @@ qemuSnapshotRevert(virDomainObj *vm, 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, -- 2.31.1
Now the cleanup label is not necessary so we can drop it. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8191733a6d..beeb012431 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1967,13 +1967,13 @@ qemuSnapshotRevert(virDomainObj *vm, if (qemuDomainHasBlockjob(vm, false)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain has active block job")); - goto cleanup; + return -1; } if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT, flags) < 0) - goto cleanup; + return -1; if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) goto endjob; @@ -2210,7 +2210,6 @@ qemuSnapshotRevert(virDomainObj *vm, endjob: qemuProcessEndJob(driver, vm); - cleanup: if (ret == 0) { qemuSnapshotSetCurrent(vm, snap); if (qemuDomainSnapshotWriteMetadata(vm, snap, -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 78 ++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index beeb012431..cc219f8b64 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1897,6 +1897,49 @@ qemuSnapshotCreateXML(virDomainPtr domain, } +static int +qemuSnapshotRevertValidate(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainSnapshotDef *snapdef, + unsigned int flags) +{ + if (!vm->persistent && + snapdef->state != VIR_DOMAIN_SNAPSHOT_RUNNING && + snapdef->state != VIR_DOMAIN_SNAPSHOT_PAUSED && + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domain needs to request run or pause to revert to inactive snapshot")); + return -1; + } + + if (virDomainSnapshotIsExternal(snap)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("revert to external snapshot not supported yet")); + return -1; + } + + if (!snap->def->dom) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, + _("snapshot '%s' lacks domain '%s' rollback info"), + snap->def->name, vm->def->name); + return -1; + } + + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { + if (vm->hasManagedSave && + !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || + snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED)) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", + _("snapshot without memory state, removal of existing managed saved state strongly recommended to avoid corruption")); + return -1; + } + } + + return 0; +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotRevertInactive(virQEMUDriver *driver, @@ -1979,41 +2022,8 @@ qemuSnapshotRevert(virDomainObj *vm, goto endjob; snapdef = virDomainSnapshotObjGetDef(snap); - if (!vm->persistent && - snapdef->state != VIR_DOMAIN_SNAPSHOT_RUNNING && - snapdef->state != VIR_DOMAIN_SNAPSHOT_PAUSED && - (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | - VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("transient domain needs to request run or pause " - "to revert to inactive snapshot")); + if (qemuSnapshotRevertValidate(vm, snap, snapdef, flags) < 0) goto endjob; - } - - if (virDomainSnapshotIsExternal(snap)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("revert to external snapshot not supported yet")); - goto endjob; - } - - if (!snap->def->dom) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, - _("snapshot '%s' lacks domain '%s' rollback info"), - snap->def->name, vm->def->name); - goto endjob; - } - - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { - if (vm->hasManagedSave && - !(snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || - snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED)) { - virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", - _("snapshot without memory state, removal of " - "existing managed saved state strongly " - "recommended to avoid corruption")); - goto endjob; - } - } config = virDomainDefCopy(snap->def->dom, driver->xmlopt, priv->qemuCaps, true); -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 95 +++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index cc219f8b64..589daaa73e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1940,6 +1940,63 @@ qemuSnapshotRevertValidate(virDomainObj *vm, } +static int +qemuSnapshotRevertPrep(virDomainMomentObj *snap, + virDomainSnapshotDef *snapdef, + virQEMUDriver *driver, + virDomainObj *vm, + virDomainDef **retConfig, + virDomainDef **retInactiveConfig) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virDomainDef) config = NULL; + g_autoptr(virDomainDef) inactiveConfig = NULL; + + config = virDomainDefCopy(snap->def->dom, + driver->xmlopt, priv->qemuCaps, true); + if (!config) + return -1; + + if (STRNEQ(config->name, vm->def->name)) { + VIR_FREE(config->name); + config->name = g_strdup(vm->def->name); + } + + if (snap->def->inactiveDom) { + inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, + driver->xmlopt, priv->qemuCaps, true); + if (!inactiveConfig) + return -1; + + if (STRNEQ(inactiveConfig->name, vm->def->name)) { + VIR_FREE(inactiveConfig->name); + inactiveConfig->name = g_strdup(vm->def->name); + } + } else { + /* Inactive domain definition is missing: + * - either this is an old active snapshot and we need to copy the + * active definition as an inactive one + * - or this is an inactive snapshot which means config contains the + * inactive definition. + */ + if (snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || + snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) { + inactiveConfig = virDomainDefCopy(snap->def->dom, + driver->xmlopt, priv->qemuCaps, true); + if (!inactiveConfig) + return -1; + } else { + inactiveConfig = g_steal_pointer(&config); + } + } + + *retConfig = g_steal_pointer(&config); + *retInactiveConfig = g_steal_pointer(&inactiveConfig); + + return 0; +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotRevertInactive(virQEMUDriver *driver, @@ -1981,7 +2038,6 @@ qemuSnapshotRevert(virDomainObj *vm, virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; int detail; - qemuDomainObjPrivate *priv = vm->privateData; int rc; g_autoptr(virDomainDef) config = NULL; g_autoptr(virDomainDef) inactiveConfig = NULL; @@ -2025,42 +2081,9 @@ qemuSnapshotRevert(virDomainObj *vm, if (qemuSnapshotRevertValidate(vm, snap, snapdef, flags) < 0) goto endjob; - config = virDomainDefCopy(snap->def->dom, - driver->xmlopt, priv->qemuCaps, true); - if (!config) + if (qemuSnapshotRevertPrep(snap, snapdef, driver, vm, + &config, &inactiveConfig) < 0) { goto endjob; - - if (STRNEQ(config->name, vm->def->name)) { - VIR_FREE(config->name); - config->name = g_strdup(vm->def->name); - } - - if (snap->def->inactiveDom) { - inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, - driver->xmlopt, priv->qemuCaps, true); - if (!inactiveConfig) - goto endjob; - - if (STRNEQ(inactiveConfig->name, vm->def->name)) { - VIR_FREE(inactiveConfig->name); - inactiveConfig->name = g_strdup(vm->def->name); - } - } else { - /* Inactive domain definition is missing: - * - either this is an old active snapshot and we need to copy the - * active definition as an inactive one - * - or this is an inactive snapshot which means config contains the - * inactive definition. - */ - if (snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING || - snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) { - inactiveConfig = virDomainDefCopy(snap->def->dom, - driver->xmlopt, priv->qemuCaps, true); - if (!inactiveConfig) - goto endjob; - } else { - inactiveConfig = g_steal_pointer(&config); - } } cookie = (qemuDomainSaveCookie *) snapdef->cookie; -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 589daaa73e..950b1a65ff 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2131,6 +2131,7 @@ qemuSnapshotRevert(virDomainObj *vm, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, detail); + virObjectEventStateQueue(driver->domainEventState, event); if (rc < 0) goto endjob; @@ -2145,6 +2146,7 @@ qemuSnapshotRevert(virDomainObj *vm, event2 = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, detail); + virObjectEventStateQueue(driver->domainEventState, event2); } else { /* Transitions 2, 5, 8 */ if (!virDomainObjIsActive(vm)) { @@ -2178,6 +2180,7 @@ qemuSnapshotRevert(virDomainObj *vm, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, detail); + virObjectEventStateQueue(driver->domainEventState, event); } if (qemuSnapshotRevertInactive(driver, vm, snap) < 0) { @@ -2197,7 +2200,6 @@ qemuSnapshotRevert(virDomainObj *vm, start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; - virObjectEventStateQueue(driver->domainEventState, event); rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, QEMU_ASYNC_JOB_START, NULL, -1, NULL, NULL, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, @@ -2211,11 +2213,13 @@ qemuSnapshotRevert(virDomainObj *vm, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, detail); + virObjectEventStateQueue(driver->domainEventState, event); if (paused) { detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; event2 = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, detail); + virObjectEventStateQueue(driver->domainEventState, event2); } } break; @@ -2261,8 +2265,6 @@ qemuSnapshotRevert(virDomainObj *vm, VIR_DOMAIN_EVENT_DEFINED, detail)); } - virObjectEventStateQueue(driver->domainEventState, event); - virObjectEventStateQueue(driver->domainEventState, event2); return ret; } -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 950b1a65ff..1d4af221e2 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2256,14 +2256,18 @@ qemuSnapshotRevert(virDomainObj *vm, ret = -1; } } - if (ret == 0 && defined && vm->persistent && - !(ret = virDomainDefSave(vm->newDef ? vm->newDef : vm->def, - driver->xmlopt, cfg->configDir))) { - detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT; - virObjectEventStateQueue(driver->domainEventState, - virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_DEFINED, - detail)); + if (ret == 0 && defined && vm->persistent) { + virDomainDef *saveDef = vm->newDef ? vm->newDef : vm->def; + + ret = virDomainDefSave(saveDef, driver->xmlopt, cfg->configDir); + + if (ret == 0) { + detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_DEFINED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + } } return ret; -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 58 +++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1d4af221e2..8ad441e41a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1997,6 +1997,40 @@ qemuSnapshotRevertPrep(virDomainMomentObj *snap, } +static int +qemuSnapshotRevertWriteMetadata(virDomainObj *vm, + virDomainMomentObj *snap, + virQEMUDriver *driver, + virQEMUDriverConfig *cfg, + bool defined) +{ + qemuSnapshotSetCurrent(vm, snap); + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->xmlopt, + cfg->snapshotDir) < 0) { + virDomainSnapshotSetCurrent(vm->snapshots, NULL); + return -1; + } + + if (defined && vm->persistent) { + int detail; + virObjectEvent *event = NULL; + virDomainDef *saveDef = vm->newDef ? vm->newDef : vm->def; + + if (virDomainDefSave(saveDef, driver->xmlopt, cfg->configDir) < 0) + return -1; + + detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_DEFINED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + } + + return 0; +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotRevertInactive(virQEMUDriver *driver, @@ -2247,28 +2281,8 @@ qemuSnapshotRevert(virDomainObj *vm, endjob: qemuProcessEndJob(driver, vm); - if (ret == 0) { - qemuSnapshotSetCurrent(vm, snap); - if (qemuDomainSnapshotWriteMetadata(vm, snap, - driver->xmlopt, - cfg->snapshotDir) < 0) { - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - ret = -1; - } - } - if (ret == 0 && defined && vm->persistent) { - virDomainDef *saveDef = vm->newDef ? vm->newDef : vm->def; - - ret = virDomainDefSave(saveDef, driver->xmlopt, cfg->configDir); - - if (ret == 0) { - detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_DEFINED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - } - } + if (ret == 0) + ret = qemuSnapshotRevertWriteMetadata(vm, snap, driver, cfg, defined); return ret; } -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8ad441e41a..5c974280af 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2276,14 +2276,11 @@ qemuSnapshotRevert(virDomainObj *vm, goto endjob; } - ret = 0; + ret = qemuSnapshotRevertWriteMetadata(vm, snap, driver, cfg, defined); endjob: qemuProcessEndJob(driver, vm); - if (ret == 0) - ret = qemuSnapshotRevertWriteMetadata(vm, snap, driver, cfg, defined); - return ret; } -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 171 ++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 73 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5c974280af..4f0e484ab5 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2031,6 +2031,99 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +static int +qemuSnapshotRevertActive(virDomainObj *vm, + virDomainSnapshotPtr snapshot, + virDomainMomentObj *snap, + virDomainSnapshotDef *snapdef, + virQEMUDriver *driver, + virQEMUDriverConfig *cfg, + virDomainDef **config, + virDomainDef **inactiveConfig, + unsigned int start_flags, + unsigned int flags) +{ + virObjectEvent *event = NULL; + virObjectEvent *event2 = NULL; + int detail; + bool defined = false; + qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; + int rc; + + start_flags |= VIR_QEMU_PROCESS_START_PAUSED; + + /* Transitions 2, 3, 5, 6, 8, 9 */ + if (virDomainObjIsActive(vm)) { + /* Transitions 5, 6, 8, 9 */ + qemuProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_START, 0); + virDomainAuditStop(vm, "from-snapshot"); + detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + } + + if (*inactiveConfig) { + virDomainObjAssignDef(vm, inactiveConfig, false, NULL); + defined = true; + } + + virDomainObjAssignDef(vm, config, true, NULL); + + /* No cookie means libvirt which saved the domain was too old to + * mess up the CPU definitions. + */ + if (cookie && + qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) + return -1; + + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, + cookie ? cookie->cpu : NULL, + QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + start_flags); + virDomainAuditStart(vm, "from-snapshot", rc >= 0); + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + if (rc < 0) + return -1; + + /* Touch up domain state. */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && + (snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED || + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { + /* Transitions 3, 6, 9 */ + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event2 = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + virObjectEventStateQueue(driver->domainEventState, event2); + } else { + /* Transitions 2, 5, 8 */ + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + return -1; + } + rc = qemuProcessStartCPUs(driver, vm, + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_START); + if (rc < 0) + return -1; + } + + return qemuSnapshotRevertWriteMetadata(vm, snap, driver, cfg, defined); +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotRevertInactive(virQEMUDriver *driver, @@ -2076,7 +2169,6 @@ qemuSnapshotRevert(virDomainObj *vm, g_autoptr(virDomainDef) config = NULL; g_autoptr(virDomainDef) inactiveConfig = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - qemuDomainSaveCookie *cookie; unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; bool defined = false; @@ -2120,81 +2212,14 @@ qemuSnapshotRevert(virDomainObj *vm, goto endjob; } - cookie = (qemuDomainSaveCookie *) snapdef->cookie; - switch ((virDomainSnapshotState) snapdef->state) { case VIR_DOMAIN_SNAPSHOT_RUNNING: case VIR_DOMAIN_SNAPSHOT_PAUSED: - start_flags |= VIR_QEMU_PROCESS_START_PAUSED; - - /* Transitions 2, 3, 5, 6, 8, 9 */ - if (virDomainObjIsActive(vm)) { - /* Transitions 5, 6, 8, 9 */ - qemuProcessStop(driver, vm, - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START, 0); - virDomainAuditStop(vm, "from-snapshot"); - detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - } - - if (inactiveConfig) { - virDomainObjAssignDef(vm, &inactiveConfig, false, NULL); - defined = true; - } - - virDomainObjAssignDef(vm, &config, true, NULL); - - /* No cookie means libvirt which saved the domain was too old to - * mess up the CPU definitions. - */ - if (cookie && - qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - goto endjob; - - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, - cookie ? cookie->cpu : NULL, - QEMU_ASYNC_JOB_START, NULL, -1, NULL, snap, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - start_flags); - virDomainAuditStart(vm, "from-snapshot", rc >= 0); - detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - if (rc < 0) - goto endjob; - - /* Touch up domain state. */ - if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && - (snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED || - (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { - /* Transitions 3, 6, 9 */ - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); - detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; - event2 = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - detail); - virObjectEventStateQueue(driver->domainEventState, event2); - } else { - /* Transitions 2, 5, 8 */ - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto endjob; - } - rc = qemuProcessStartCPUs(driver, vm, - VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START); - if (rc < 0) - goto endjob; - } - break; + ret = qemuSnapshotRevertActive(vm, snapshot, snap, snapdef, + driver, cfg, + &config, &inactiveConfig, + start_flags, flags); + goto endjob; case VIR_DOMAIN_SNAPSHOT_SHUTDOWN: case VIR_DOMAIN_SNAPSHOT_SHUTOFF: -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 4f0e484ab5..6b300d808a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2126,9 +2126,9 @@ qemuSnapshotRevertActive(virDomainObj *vm, /* The domain is expected to be locked and inactive. */ static int -qemuSnapshotRevertInactive(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap) +qemuSnapshotInternalRevertInactive(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap) { size_t i; @@ -2242,7 +2242,7 @@ qemuSnapshotRevert(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } - if (qemuSnapshotRevertInactive(driver, vm, snap) < 0) { + if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { qemuDomainRemoveInactive(driver, vm); goto endjob; } -- 2.31.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 148 ++++++++++++++++++++++----------------- 1 file changed, 83 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6b300d808a..1a32b15f51 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2153,6 +2153,84 @@ qemuSnapshotInternalRevertInactive(virQEMUDriver *driver, } +static int +qemuSnapshotRevertInactive(virDomainObj *vm, + virDomainSnapshotPtr snapshot, + virDomainMomentObj *snap, + virQEMUDriver *driver, + virQEMUDriverConfig *cfg, + virDomainDef **inactiveConfig, + unsigned int start_flags, + unsigned int flags) +{ + virObjectEvent *event = NULL; + virObjectEvent *event2 = NULL; + int detail; + bool defined = false; + int rc; + + /* Transitions 1, 4, 7 */ + /* Newer qemu -loadvm refuses to revert to the state of a snapshot + * created by qemu-img snapshot -c. If the domain is running, we + * must take it offline; then do the revert using qemu-img. + */ + + if (virDomainObjIsActive(vm)) { + /* Transitions 4, 7 */ + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_START, 0); + virDomainAuditStop(vm, "from-snapshot"); + detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STOPPED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + } + + if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { + qemuDomainRemoveInactive(driver, vm); + return -1; + } + + if (*inactiveConfig) { + virDomainObjAssignDef(vm, inactiveConfig, false, NULL); + return -1; + } + + if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + /* Flush first event, now do transition 2 or 3 */ + bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; + + start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; + + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, + QEMU_ASYNC_JOB_START, NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + start_flags); + virDomainAuditStart(vm, "from-snapshot", rc >= 0); + if (rc < 0) { + qemuDomainRemoveInactive(driver, vm); + return -1; + } + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); + virObjectEventStateQueue(driver->domainEventState, event); + if (paused) { + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event2 = virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + virObjectEventStateQueue(driver->domainEventState, event2); + } + } + + return qemuSnapshotRevertWriteMetadata(vm, snap, driver, cfg, defined); +} + + int qemuSnapshotRevert(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -2162,15 +2240,10 @@ qemuSnapshotRevert(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; virDomainSnapshotDef *snapdef; - virObjectEvent *event = NULL; - virObjectEvent *event2 = NULL; - int detail; - int rc; g_autoptr(virDomainDef) config = NULL; g_autoptr(virDomainDef) inactiveConfig = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; - bool defined = false; virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | @@ -2224,64 +2297,11 @@ qemuSnapshotRevert(virDomainObj *vm, case VIR_DOMAIN_SNAPSHOT_SHUTDOWN: case VIR_DOMAIN_SNAPSHOT_SHUTOFF: case VIR_DOMAIN_SNAPSHOT_CRASHED: - /* Transitions 1, 4, 7 */ - /* Newer qemu -loadvm refuses to revert to the state of a snapshot - * created by qemu-img snapshot -c. If the domain is running, we - * must take it offline; then do the revert using qemu-img. - */ - - if (virDomainObjIsActive(vm)) { - /* Transitions 4, 7 */ - qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_START, 0); - virDomainAuditStop(vm, "from-snapshot"); - detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - } - - if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm); - goto endjob; - } - - if (inactiveConfig) { - virDomainObjAssignDef(vm, &inactiveConfig, false, NULL); - defined = true; - } - - if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | - VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { - /* Flush first event, now do transition 2 or 3 */ - bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; - - start_flags |= paused ? VIR_QEMU_PROCESS_START_PAUSED : 0; - - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - QEMU_ASYNC_JOB_START, NULL, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - start_flags); - virDomainAuditStart(vm, "from-snapshot", rc >= 0); - if (rc < 0) { - qemuDomainRemoveInactive(driver, vm); - goto endjob; - } - detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - detail); - virObjectEventStateQueue(driver->domainEventState, event); - if (paused) { - detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; - event2 = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - detail); - virObjectEventStateQueue(driver->domainEventState, event2); - } - } - break; + ret = qemuSnapshotRevertInactive(vm, snapshot, snap, + driver, cfg, + &inactiveConfig, + start_flags, flags); + goto endjob; case VIR_DOMAIN_SNAPSHOT_PMSUSPENDED: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -2301,8 +2321,6 @@ qemuSnapshotRevert(virDomainObj *vm, goto endjob; } - ret = qemuSnapshotRevertWriteMetadata(vm, snap, driver, cfg, defined); - endjob: qemuProcessEndJob(driver, vm); -- 2.31.1
On a Thursday in 2021, Pavel Hrdina wrote:
Pavel Hrdina (14): qemu_snapshot: revert: fix incorrect jump to cleanup qemu_snapshot: revert: drop unused variable qemu_snapshot: revert: use g_autoptr qemu_snapshot: revert: jump to endjob instead of calling qemuProcessEndJob qemu_snapshot: revert: remove cleanup label qemu_snapshot: revert: move validation to separate function qemu_snapshot: revert: move config prepare code to separate function qemu_snapshot: revert: emit event right after they are created qemu_snapshot: revert: refactor cleanup section qemu_snapshot: revert: move saving metadata to separate function qemu_snapshot: revert: save metadata within qemu process job qemu_snapshot: revert: move active snapshot revert to separate function qemu_snapshot: revert: rename qemuSnapshotRevertInactive qemu_snapshot: revert: move inactive snapshot to separate function
src/qemu/qemu_snapshot.c | 554 ++++++++++++++++++++++----------------- 1 file changed, 320 insertions(+), 234 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Pavel Hrdina