GNOME Bugzilla – Bug 685846
Live VMs gets deleted automatically even on forced shutdowns
Last modified: 2016-03-31 14:01:11 UTC
While its desirable to automatically delete live VMs on normal shutdown if nothing is installed during the live run, its pprobably not the desired behavior when VM is forced shutdown. Attaching patches that fix this issue.
Created attachment 226146 [details] [review] libvirt-machine: API to indicate forced shutdown Add a public boolean field to indicate if last shutdown was forced.
Created attachment 226147 [details] [review] vm-creator: Correctly handle forced shutdowns Don't do any post-installation checks on a machine if its forced to shutdown. This is not only the right thing to do but it also solves the issue of live VMs getting automatically deleted on forced shutdown.
Review of attachment 226146 [details] [review]: Shouldn't this be an additional MachineState rather than adding a boolean to update every time the state change?
Review of attachment 226147 [details] [review]: ::: src/vm-creator.vala @@ +97,3 @@ } + if (machine.state == Machine.MachineState.STOPPED && machine.last_shutdown_forced) if (machine.state == Machine.MachineState.FORCED_STOPPED) return; would look better imo.
Created attachment 226174 [details] [review] libvirt-machine: API to indicate forced shutdown Add a new machine state enum to indicate if last shutdown was forced.
Created attachment 226175 [details] [review] vm-creator: Correctly handle forced shutdowns Don't do any post-installation checks on a machine if its forced to shutdown. This is not only the right thing to do but it also solves the issue of live VMs getting automatically deleted on forced shutdown.
Created attachment 226176 [details] [review] libvirt-machine: More use of new is_on() predicate We don't tell user to reboot the box if machine is exactly in 'stopped' state. We probably want to apply that to all off states. A bit unrelated to his bug but small one so too lazy to file a new one for it. :)
Review of attachment 226174 [details] [review]: ::: src/machine.vala @@ +35,3 @@ SAVED, + SLEEPING, + FORCE_STOPPED nitpick, it would be nicer to add it close to STOPPED
Review of attachment 226174 [details] [review]: ACK from me if Marc-André's comments are addressed.
Review of attachment 226175 [details] [review]: Looks good.
Review of attachment 226176 [details] [review]: ::: src/libvirt-machine.vala @@ +534,2 @@ this.notify["state"].connect (() => { + if (!is_on ()) ACK to this one @@ +596,3 @@ ulong state_id = 0; state_id = this.notify["state"].connect (() => { + if (!is_on ()) { This one is more surprising, it's meant to catch the end of the 'shutdown' call further down this function, so STOPPED seems like the right check to have.
Attachment 226174 [details] pushed as 7e86dde - libvirt-machine: API to indicate forced shutdown Attachment 226175 [details] pushed as 6551a33 - vm-creator: Correctly handle forced shutdowns Attachment 226176 [details] pushed as 8a7b1ab - libvirt-machine: More use of new is_on() predicate
Created attachment 227728 [details] [review] Handle screenshots correctly on FORCE_STOPPED This should be handled just like STOPPED. Without this we get shaded screenshots instead of black screenshots when you force-stop a box.
reopening due to regression, see patch above.
Review of attachment 227728 [details] [review]: ACK
Attachment 227728 [details] pushed as 526cd07 - Handle screenshots correctly on FORCE_STOPPED
*** Bug 688263 has been marked as a duplicate of this bug. ***