On Tue, Nov 16, 2021 at 03:30:02PM +0100, Peter Krempa wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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.
True, originally I had the `was_stopped = true;` after the if part, not
in the else part and it was correct, no idea what made me to change the
code. I'll fix it.
Thanks,
Pavel
> > + }
> >
> > - if (inactiveConfig) {
> > - virDomainObjAssignDef(vm, inactiveConfig, false, NULL);
> > - inactiveConfig = NULL;
> > - defined = true;
> > - }
>
> The rest looks good.
>
Attachment:
signature.asc
Description: PGP signature
