Created attachment 222683 [details] [review]
libvirt-machine: Only stop running or paused machines

If domain is already stopped, don't try to stop it. Doing so will only
trigger an error and the following code to delete the volume won't be
executed, resulting in a leak of storage volume.

You can reproduce the issue easily by hitting 'Cancel' at the review
page of wizard.

Review of attachment 222683 [details] [review]:

::: src/libvirt-machine.vala
@@ +394,3 @@
                     // Ensure that the domain is stopped before we touch any data
+                    if (state == MachineState.RUNNING || state == MachineState.PAUSED)
+                        domain.stop (0);

I'd rather we do the stop unconditionally and just catch the potential exceptions. If stop() raise an exception in another case we haven't thought about, this new code will get the same leak.

I agree, lets just put each operation in its own try/catch.

Created attachment 222794 [details] [review]
libvirt-machine: Don't leak volume on failed stop or delete

If domain is already stopped, attempting to stop it will fail. Put all
clean-up operations related to deletion in their own try/catch block so
failure of one doesn't mean the others wont be executed.

Review of attachment 222794 [details] [review]:

::: src/libvirt-machine.vala
@@ +401,2 @@
                 } catch (GLib.Error err) {
                     warning (err.message);

Dunno if we want to always warn as this will happen in legit cases

(In reply to comment #5)
> Review of attachment 222794 [details] [review]:
> 
> ::: src/libvirt-machine.vala
> @@ +401,2 @@
>                  } catch (GLib.Error err) {
>                      warning (err.message);
> 
> Dunno if we want to always warn as this will happen in legit cases

How do we filter? Ignore if domain was in running or paused state?

(In reply to comment #6)
> (In reply to comment #5)
> > Review of attachment 222794 [details] [review] [details]:
> > 
> > ::: src/libvirt-machine.vala
> > @@ +401,2 @@
> >                  } catch (GLib.Error err) {
> >                      warning (err.message);
> > 
> > Dunno if we want to always warn as this will happen in legit cases
> 
> How do we filter? Ignore if domain was in running or paused state?

Or just always log to debug and remove the warning ?

Created attachment 222801 [details] [review]
libvirt-machine: Don't leak volume on failed stop or delete

V2: Change a warning to debug

Review of attachment 222801 [details] [review]:

Looks good to me

Attachment 222801 [details] pushed as d78d431 - libvirt-machine: Don't leak volume on failed stop or delete