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

[Pavel Hrdina]

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 at redhat.com>
> > ---
> >  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.
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20211122/0fd3b79a/attachment-0001.sig>