GNOME Bugzilla – Bug 685383
Clean up the S3 sleep mode code after branching
Last modified: 2016-03-31 14:00:31 UTC
Qemu/Libvirt added a new S3 sleep mode with new events and stuff, and boxes gets really confused when it happes to it. We need to add support to it in boxes. Ideally we should do so without a hard dependency on the new libvirt-glib.
Created attachment 225677 [details] [review] Add wrappers for virDomainPMWakeup
Created attachment 225678 [details] [review] Add new SLEEP mode for PMSUSPEND We do this without any hard dependency on the new symbols in libvirt-glib so that we can still build with the old libvirt.
This is not really tested yet in sleep mode, but seems to be right. Just wanted to get it in here for comments.
Created attachment 225784 [details] [review] Add new SLEEP mode for PMSUSPEND We do this without any hard dependency on the new symbols in libvirt-glib so that we can still build with the old libvirt.
New version that fixes a crash in the pmsuspended signal handler. This works for me, but one time i did get a "disconnect" while connecting to a box that needed wakeup. It did wake it up though, so a second click worked. Might be some unrelated disconnect issue as it only happened once.
Review of attachment 225677 [details] [review]: This one's libvirt-glib patch, it has been reviewed on libvirt ML and pushed
Review of attachment 225784 [details] [review]: I've played a bit with this, and seemed to have rough edges in qemu/libvirt :-/ I tried S3 with a f18 livecd, and when I suspend it, the VM gets destroyed, which is not what I expect. ::: src/libvirt-machine.vala @@ +120,3 @@ domain.resumed.connect (() => { state = MachineState.RUNNING; }); domain.stopped.connect (() => { + if (Signal.get_invocation_hint (this.domain).detail == Quark.from_string ("saved")) This change seems unrelated/not really useful? @@ +377,3 @@ } + if (state != DomainState.RUNNING && state != DomainState.PAUSED && state != DomainStatePMSUSPENDED) this could be a small helper as this appears 3 times in the diff ::: src/machine.vala @@ +34,3 @@ PAUSED, + SAVED, + SLEEP SLEEPING would be more consistent with RUNNING
Review of attachment 225784 [details] [review]: ::: src/libvirt-machine.vala @@ +120,3 @@ domain.resumed.connect (() => { state = MachineState.RUNNING; }); domain.stopped.connect (() => { + if (Signal.get_invocation_hint (this.domain).detail == Quark.from_string ("saved")) Yeah, its unrelated. I just saw it when reading the C code. It avoids creating an unnecessary closure to access the local va "domain" rather than just passing in "this" as the user data and getting this.domain. ::: src/machine.vala @@ +34,3 @@ PAUSED, + SAVED, + SLEEP True.
Review of attachment 225784 [details] [review]: ::: src/libvirt-machine.vala @@ +377,3 @@ } + if (state != DomainState.RUNNING && state != DomainState.PAUSED && state != DomainStatePMSUSPENDED) Its not actually 3 times. One is DomainState.* and two are MachineState.*. Not sure what to call the common state though...
Created attachment 225810 [details] [review] Avoid unnecessary closure By accessing this.domain rather than the local var "domain" in the function we can avoid passing an unnecessary closure object to the signal handler.
Created attachment 225811 [details] [review] Add new SLEEPING mode for PMSUSPEND We do this without any hard dependency on the new symbols in libvirt-glib so that we can still build with the old libvirt.
Created attachment 225812 [details] [review] Add Machine.is_on() helper This simplifies some code in LibvirtMachine.
Review of attachment 225784 [details] [review]: ::: src/libvirt-machine.vala @@ +377,3 @@ } + if (state != DomainState.RUNNING && state != DomainState.PAUSED && state != DomainStatePMSUSPENDED) Ah my bad, as usual didn't pay close enough attention :(
Review of attachment 225810 [details] [review]: ACK, thanks for explaining what this change is doing, this was not obvious at all to me ;)
Review of attachment 225812 [details] [review]: Looks good, thanks for doing that change
Review of attachment 225811 [details] [review]: I'm still getting the issue that my live cd VM goes away when I suspend it (but there's a similar bug with force shutdown, so this probably can be addressed separately). I'd prefer if we got rid of the compat code as soon as possible (when we start 3.7 I guess). Looks good otherwise, just a small question ::: src/libvirt-machine.vala @@ +573,3 @@ + yield ((BoxesGVir.Domain)domain).wakeup_async (0, null); + else + throw new Boxes.Error.INVALID ("Wakeup not supported"); What happens in this case? Does the user have any way of waking up the VM?
Attachment 225810 [details] pushed as 4345c6e - Avoid unnecessary closure Attachment 225811 [details] pushed as 39d2544 - Add new SLEEPING mode for PMSUSPEND Attachment 225812 [details] pushed as ee4dbee - Add Machine.is_on() helper
Review of attachment 225811 [details] [review]: Yeah, we should drop the compat code as soon as we branch. We should probably keep the bug open for that. ::: src/libvirt-machine.vala @@ +573,3 @@ + yield ((BoxesGVir.Domain)domain).wakeup_async (0, null); + else + throw new Boxes.Error.INVALID ("Wakeup not supported"); This would happen if you installed a new qemu and libvirt with S3 support but did not build boxes against it, and then the guest decided to go to sleep. There is not much to do in this case from inside boxes. But since you installed the new libvirt to enable this support you could possibly use virt-manager to wake it up. Or, just use a boxes build that matches your libvirt build.
Reopening to keep track of the fact that this needs post-branch cleanup.
closed as we cleaned up the config stuff.