Re: [libvirt PATCH 03/11] qemu_snapshot: revert: drop unused loadvm code

On Mon, Nov 15, 2021 at 17:22:46 +0100, Pavel Hrdina wrote: > Now that we always restart QEMU process the loadvm code is unused and > can be dropped. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/qemu/qemu_snapshot.c | 96 +++++++++++----------------------------- > 1 file changed, 26 insertions(+), 70 deletions(-) > > diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c > index 661f74146c..251a0e5cfa 100644 > --- a/src/qemu/qemu_snapshot.c > +++ b/src/qemu/qemu_snapshot.c > @@ -1982,11 +1982,6 @@ qemuSnapshotRevert(virDomainObj *vm, > start_flags |= VIR_QEMU_PROCESS_START_PAUSED; > > /* Transitions 2, 3, 5, 6, 8, 9 */ > - /* When using the loadvm monitor command, qemu does not know > - * whether to pause or run the reverted domain, and just stays > - * in the same state as before the monitor command, whether > - * that is paused or running. We always pause before loadvm, > - * to have finer control. */ > if (virDomainObjIsActive(vm)) { > /* Transitions 5, 6, 8, 9 */ > qemuProcessStop(driver, vm, > @@ -1998,78 +1993,39 @@ qemuSnapshotRevert(virDomainObj *vm, > VIR_DOMAIN_EVENT_STOPPED, > detail); > virObjectEventStateQueue(driver->domainEventState, event); > - goto load; So this jumped ... > - > - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { > - /* Transitions 5, 6 */ > - if (qemuProcessStopCPUs(driver, vm, > - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, > - QEMU_ASYNC_JOB_START) < 0) > - goto endjob; > - if (!virDomainObjIsActive(vm)) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("guest unexpectedly quit")); > - goto endjob; > - } > - } > - > - if (qemuDomainObjEnterMonitorAsync(driver, vm, > - QEMU_ASYNC_JOB_START) < 0) > - goto endjob; > - rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); (Don't forget do delete the monitor code for 'loadvm' ;)) > - if (qemuDomainObjExitMonitor(driver, vm) < 0) > - goto endjob; > - if (rc < 0) { > - /* XXX resume domain if it was running before the > - * failed loadvm attempt? */ > - goto endjob; > - } > - > - virCPUDefFree(priv->origCPU); > - priv->origCPU = g_steal_pointer(&origCPU); > - > - if (cookie && !cookie->slirpHelper) > - priv->disableSlirp = true; > - > - if (inactiveConfig) { > - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); > - inactiveConfig = NULL; > - defined = true; > - } > } else { > /* Transitions 2, 3 */ > - load: ... here. > was_stopped = true; Which meant that 'was_stopped' was set to true when reverting when the VM was killed, which is not happening after this patch. The commit message is claiming pure dead code removal, thus this must be fixed or justified.

> + } > > - if (inactiveConfig) { > - virDomainObjAssignDef(vm, inactiveConfig, false, NULL); > - inactiveConfig = NULL; > - defined = true; > - } The rest looks good.