GNOME Bugzilla – Bug 687626
Can't start VM if restore fails
Last modified: 2016-03-31 13:55:00 UTC
I had an instance of a VM that was saved, but there was an error during restore. This just errored out and got me back to the collection view, where clicking on it again did the same. In virt-manager i get a dialog asking if i want to remove the saved data and start up from scrach. We need something like that.
Fwiw, it is currently very easy to get in such a state by saving with qemu-1.2.0-18 and trying to restore with qemu-1.2.0-19
(fedora 18 packages)
Same as https://bugzilla.redhat.com/show_bug.cgi?id=892745 ?
Its kinda the same. That bug describes the underlying issue, i.e. that libvirt can't handle the old save file. However, there is a follow-on bug in gnome-boxes where we in this situation just silently throw away the error and don't let the user work around this in any way by removing the old save file.
(In reply to comment #4) > Its kinda the same. That bug describes the underlying issue, i.e. that libvirt > can't handle the old save file. However, there is a follow-on bug in > gnome-boxes where we in this situation just silently throw away the error and > don't let the user work around this in any way by removing the old save file. We'll need some UI for that since simply removing saved state can cause dataloss. Its just like force shutdown.
*** Bug 692062 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > Fwiw, it is currently very easy to get in such a state by saving with > qemu-1.2.0-18 and trying to restore with qemu-1.2.0-19 As noted in https://bugzilla.redhat.com/show_bug.cgi?id=892745#c11 , saving with qemu-1.2.0-18 and restoring with the qemu-1.2.2-1 fedora package is working (restoring with the previous qemu build, qemu-1.2.0-25 was still broken).
Created attachment 237179 [details] [review] Allow ignoring saved state if restore fails
Review of attachment 237179 [details] [review]: Looks good otherwise. ::: src/app.vala @@ +897,3 @@ + if (e is Boxes.Error.RESTORE_FAILED && + !(Machine.ConnectFlags.IGNORE_SAVED_STATE in flags)) { + App.app.notificationbar.display_for_action (_("'%s' could not be restored from disk\nTry without saved state?").printf (machine.name), I think these lines go beyond 120 columns limit. Probably good idea to use some temporary variables. ::: src/libvirt-machine-properties.vala @@ +443,3 @@ machine.state == Machine.MachineState.FORCE_STOPPED) { debug ("'%s' stopped.", machine.name); + machine.start.begin (0, (obj, res) => { is this related? ::: src/libvirt-machine.vala @@ +23,3 @@ } + public override async void connect_display (Machine.ConnectFlags flags) throws GLib.Error { Should we have flags optional here and in start() below as well (just like in app)? At least the call from machine.vala would benefit from it and won't need to be changed for this patch. @@ +80,2 @@ disconnect_display (); + connect_display.begin (Machine.ConnectFlags.NONE); forgot to use the passed flags argument? @@ +507,3 @@ status = _("Starting %s").printf (name); + try { + yield domain.start_async ((Machine.ConnectFlags.IGNORE_SAVED_STATE in flags) ? GVir.DomainStartFlags.FORCE_BOOT : 0, null); * another very long line, would be more readable to use a variable for the flags. * using the ConnectFlags like this seems like we actually wanted to use boolean but didn't. Perhaps it would be better to have enums be equivalent? I mean: enum ConnectFlags { NONE = GVir.DomainStartFlags.NONE, IGNORE_SAVED_STATE = GVir.DomainStartFlags.FORCE_BOOT, }
Review of attachment 237179 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +443,3 @@ machine.state == Machine.MachineState.FORCE_STOPPED) { debug ("'%s' stopped.", machine.name); + machine.start.begin (0, (obj, res) => { Yes, machine.start got a new flags argument ::: src/libvirt-machine.vala @@ +23,3 @@ } + public override async void connect_display (Machine.ConnectFlags flags) throws GLib.Error { I tried that, but it didn't really work out all that well for the async case as it needed the callback which was at the end of the argument list. It only works for calls that ignore the results, so it seemed kinda lame. @@ +80,2 @@ disconnect_display (); + connect_display.begin (Machine.ConnectFlags.NONE); I don't think we want to pass the old flags on reconnect really. Seems dangerous, since they might cause data loss. @@ +507,3 @@ status = _("Starting %s").printf (name); + try { + yield domain.start_async ((Machine.ConnectFlags.IGNORE_SAVED_STATE in flags) ? GVir.DomainStartFlags.FORCE_BOOT : 0, null); Well, we want a flag as a boolean because it makes callers more obvious (who knows what "true" does?), and its extensible. sharing the enum values seems a bit weird. All Machine implementations don't use libvirt. I think its better to move the conversion to a helper function.
Review of attachment 237179 [details] [review]: ::: src/libvirt-machine-properties.vala @@ +443,3 @@ machine.state == Machine.MachineState.FORCE_STOPPED) { debug ("'%s' stopped.", machine.name); + machine.start.begin (0, (obj, res) => { ah its the flags, better make it explicit then: Flags.NONE rather than 0.
Created attachment 237192 [details] [review] Allow ignoring saved state if restore fails
Review of attachment 237192 [details] [review]: Looks good now. ACK
Attachment 237192 [details] pushed as 22b8161 - Allow ignoring saved state if restore fails