Re: [libvirt] [PATCH 7/7] qemu: snapshot: Correctly revert snapshots in PMSUSPENDED state

On 07/18/2014 10:11 AM, Peter Krempa wrote: > PMSUSPENDED state implies the qemu process running, so we should treat > it that way. This increases the possible state space of transitions that > may happen during the snapshot revert so this patch documents them as > well. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1079162 > --- > src/qemu/qemu_driver.c | 67 ++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 48 insertions(+), 19 deletions(-) I think this needs some rework - current qemu treats it as reverting to a pm wakeup event, and the guest will start running again (unless the user requested to revert as paused). > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 1f98f4a..f93e0fd 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -14052,22 +14052,29 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > virDomainDefPtr config = NULL; > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > - int oldState = vm->state.state; > + int oldState; > > virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | > VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1); > > /* We have the following transitions, which create the following events: > - * 1. inactive -> inactive: none > - * 2. inactive -> running: EVENT_STARTED > - * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED > - * 4. running -> inactive: EVENT_STOPPED > - * 5. running -> running: none > - * 6. running -> paused: EVENT_PAUSED > - * 7. paused -> inactive: EVENT_STOPPED > - * 8. paused -> running: EVENT_RESUMED > - * 9. paused -> paused: none > + * 1. inactive -> inactive: none > + * 2. inactive -> running: EVENT_STARTED > + * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED > + * 4. running -> inactive: EVENT_STOPPED > + * 5. running -> running: none > + * 6. running -> paused: EVENT_PAUSED > + * 7. paused -> inactive: EVENT_STOPPED > + * 8. paused -> running: EVENT_RESUMED > + * 9. paused -> paused: none > + * 10. pmsuspended -> pmsuspended: none this state is not possible with current qemu > + * 11. pmsuspended -> inactive: EVENT_STOPPED > + * 12. pmsuspended -> running: EVENT_RESUMED > + * 13. pmsuspended -> paused: EVENT_PAUSED > + * 14. inactive -> pmsuspended: EVENT_STARTED, EVENT_PMSUSPENDED > + * 15. paused -> pmsuspended: EVENT_PMSUSPENDED > + * 16. running -> pmsuspended: EVENT_PMSUSPENDED and these three states are not possible > * Also, several transitions occur even if we fail partway through, > * and use of FORCE can cause multiple transitions. > */ > @@ -14075,6 +14082,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > if (!(vm = qemuDomObjFromSnapshot(snapshot))) > return -1; > > + oldState = vm->state.state; > + > cfg = virQEMUDriverGetConfig(driver); > > if (virDomainRevertToSnapshotEnsureACL(snapshot->domain->conn, vm->def) < 0) > @@ -14127,6 +14136,14 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > } > } > > + if (snap->def->state == VIR_DOMAIN_PMSUSPENDED && > + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING || > + flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > + _("snapshot of a VM in PMSUSPENDED state cannot be " > + "reverted to a running or paused state")); actually, I think that we need to tackle it differently - we can't CREATE a snapshot in the pmsuspended state (just as we can't migrate in that state - the mere act of migration is a wakeup event). So, if you eliminate all snapshots in the pmsuspended state as invalid, the set of state transitions is smaller.