On 3/27/19 7:53 AM, Nikolay Shirokovskiy wrote: > ping > > On 13.02.2019 19:55, John Ferlan wrote: >> >> >> On 2/8/19 2:52 AM, Nikolay Shirokovskiy wrote: >>> Patches 1 and 2 are already Reviewed-by: John. Patch 3 needs Peter comments. >>> >> >> Right - feel free to add my : >> >> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx> >> >> to the first 2 patches for sure. >> >> To help push this along, Peter is again CC'd and of importance is the v1 >> review of patch3: >> >> https://www.redhat.com/archives/libvir-list/2018-October/msg00831.html >> >> where I noted a specific commit and case to be considered. >> To dredge up more context, John is talking about Peter's commit: commit f569b87f5193a69c50ca714734013cc4f82250f1 Author: Peter Krempa <pkrempa@xxxxxxxxxx> Date: Mon Oct 8 19:38:44 2012 +0200 snapshot: qemu: Add support for external checkpoints Which added this chunk: > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 9358ad99ed..128b98ee22 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -1673,6 +1673,9 @@ static int qemudDomainSuspend(virDomainPtr dom) { > if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) { > reason = VIR_DOMAIN_PAUSED_MIGRATION; > eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED; > + } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) { > + reason = VIR_DOMAIN_PAUSED_SNAPSHOT; > + eventDetail = -1; /* don't create lifecycle events when doing snapshot */ > } else { > reason = VIR_DOMAIN_PAUSED_USER; > eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; > @@ -1687,9 +1690,12 @@ static int qemudDomainSuspend(virDomainPtr dom) { > if (qemuProcessStopCPUs(driver, vm, reason, QEMU_ASYNC_JOB_NONE) < 0) { > goto endjob; > } > - event = virDomainEventNewFromObj(vm, > - VIR_DOMAIN_EVENT_SUSPENDED, > - eventDetail); > + > + if (eventDetail >= 0) { > + event = virDomainEventNewFromObj(vm, > + VIR_DOMAIN_EVENT_SUSPENDED, > + eventDetail); > + } > } > if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) > goto endjob; Which disables sending a VIR_DOMAIN_EVENT_SUSPENDED event when we suspend the VM as an implementation detail of creating an external snapshot checkpoint. Nikolay's series removes that case, so AFAICT checkpoints will now send SUSPENDED events. Current git doesn't perform the same event skippage in the managed save case (where SUSPENDED event is emitted), or the internal snapshot case (where SUSPEND+RESUME are emitted when taking a live snapshot). I can't think of any particular reason why the checkpoint case needs to be special here. Maybe that's an argument for fixing all of these cases, though virt-manager which uses internal snapshots has had no problem with SUSPEND/RESUME events in this area so there's some proof apps aren't going to choke on a similar case IMO patches have been reviewed for almost 2 months, no one has objected, so push em :) Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
