Created attachment 225858 [details] [review]
Continue saved installations on launch

This fixes a regression introduced by 4f6f421.

Review of attachment 225858 [details] [review]:

::: src/vm-creator.vala
@@ +88,3 @@
+        if (machine.state != Machine.MachineState.STOPPED &&
+            machine.state != Machine.MachineState.SAVED &&
+            machine.state != Machine.MachineState.UNKNOWN)

Maybe if (machine.state == Machine.MachineState.RUNNING) return;
would be more future-proof? (a SLEEPING state is being added)

@@ +99,3 @@
         }
 
+        if (machine.state == Machine.MachineState.SAVED) {

Is this change part of the regression fix? or is it just cosmetic?

Review of attachment 225858 [details] [review]:

::: src/vm-creator.vala
@@ +88,3 @@
+        if (machine.state != Machine.MachineState.STOPPED &&
+            machine.state != Machine.MachineState.SAVED &&
+            machine.state != Machine.MachineState.UNKNOWN)

We are only interested in particular states, UKNOWN is included because we treat that as shutdown.

@@ +99,3 @@
         }
 
+        if (machine.state == Machine.MachineState.SAVED) {

its part of it.

Review of attachment 225858 [details] [review]:

Sorry, I don't understand at all why the 2nd hunk is needed :(

::: src/vm-creator.vala
@@ +88,3 @@
+        if (machine.state != Machine.MachineState.STOPPED &&
+            machine.state != Machine.MachineState.SAVED &&
+            machine.state != Machine.MachineState.UNKNOWN)

hmm is that a case where the new (!machine.is_on()) could be used?

@@ +99,3 @@
         }
 
+        if (machine.state == Machine.MachineState.SAVED) {

I think I'm missing something, I'm reviewing on the assumption that this code is running at boxes startup, in which case domain.get_saved() and MachineState.SAVED seems to be equivant from my reading of LibvirtMachine().
Is there another corner case we want to catch where "stopped::saved" has been emitted but get_saved() is false?

In short, what am I missing ? ;)

Review of attachment 225858 [details] [review]:

::: src/vm-creator.vala
@@ +88,3 @@
+        if (machine.state != Machine.MachineState.STOPPED &&
+            machine.state != Machine.MachineState.SAVED &&
+            machine.state != Machine.MachineState.UNKNOWN)

don't think so. We are not interested in suspended machines AFAIK. Perhaps you are correct here but I think its safer to not change the logic too much here and simply add the new state to the check.

@@ +99,3 @@
         }
 
+        if (machine.state == Machine.MachineState.SAVED) {

Sorry, i misremembered. You are correct that its not exactly part of the change. Since we now rely on machine state to be correctly set to MachineState.SAVED for saved machine, in the code above, i don't see any problems with relying on that in here at the same time.

Review of attachment 225858 [details] [review]:

::: src/vm-creator.vala
@@ +88,3 @@
+        if (machine.state != Machine.MachineState.STOPPED &&
+            machine.state != Machine.MachineState.SAVED &&
+            machine.state != Machine.MachineState.UNKNOWN)

machine.is_on() would not really change the logic, it expands to (state == RUNNING) || (state == PAUSED) || (state == SLEEPING), which is the same as (state != STOPPED) && (state != SAVED) && (state != UNKNOWN) as MachineState can only have 6 values.

What matters is what this test is trying to achieve. If we want to test if qemu is running/if there's an active and an inactive libvirt config for this domain, then we should use is_on, otherwise we can keep the test as it is now.

Created attachment 226044 [details] [review]
Continue saved installations on launch

v2: make use of new Machine.is_on()

Review of attachment 226044 [details] [review]:

ACK (assuming you think using is_on() is the right function to use after my comments, ie don't do this only because I insist a bit ;)

Attachment 226044 [details] pushed as 7e8730c - Continue saved installations on launch