Created attachment 202443 [details] [review]
Fix removal of libvirt machines

Because of cyclic and leaked references, libvirt machines weren't
actually getting entirely removed. As a result boxes kept asking the
inexistent domains for stats every second and ended-up flooding the
console with warnings.

Review of attachment 202443 [details] [review]:

::: src/app.vala
@@ +97,3 @@
         connection.domain_removed.connect ((connection, domain) => {
+            var machine = domain.get_data<unowned LibvirtMachine> ("machine");
+            machine.set_stats_enable (false);

if the leaks were gone, I suppose this wouldn't be necessary

perhaps you can just make a patch with this set_stats_enable (false); add a FIXME:  refcount object not destroyed or something. I am not sure about the rest of the changes now,

Created attachment 202450 [details] [review]
Fix removal of libvirt machines

Because of cyclic and leaked references, libvirt machines weren't
actually getting entirely removed. As a result boxes kept asking the
inexistent domains for stats every second and ended-up flooding the
console with warnings.

Review of attachment 202450 [details] [review]:

::: src/app.vala
@@ +97,3 @@
         connection.domain_removed.connect ((connection, domain) => {
             var machine = domain.get_data<LibvirtMachine> ("machine");
+            machine.set_stats_enable (false);

same remark, if the leaks were gone, this wouldn't be needed.

(In reply to comment #4)
> Review of attachment 202450 [details] [review]:
> 
> ::: src/app.vala
> @@ +97,3 @@
>          connection.domain_removed.connect ((connection, domain) => {
>              var machine = domain.get_data<LibvirtMachine> ("machine");
> +            machine.set_stats_enable (false);
> 
> same remark, if the leaks were gone, this wouldn't be needed.

This is needed to get rid of the ref that a previous machine.set_stats_enable (true) call from within LibvirtMachine constructor creates. Thats why I left 'cyclic ref' in the commit log.

Created attachment 202852 [details] [review]
Fix removal of libvirt machines

Because of some leaked references, libvirt machines and associated
periodic updates weren't actually getting removed. Boxes kept asking
inexistent domains for stats every second and ended-up flooding the
console with warnings.

Review of attachment 202852 [details] [review]:

2 comments, otherwise ack

::: src/libvirt-machine.vala
@@ +330,3 @@
         }
 
         set_stats_enable (false);

can you also make set_stats_enable private?

::: src/remote-machine.vala
@@ +84,3 @@
 
+    public override void delete (bool by_user = true) {
+        assert (by_user);

I prefer return_if_fail() which at least doesn't abort your program and let's you finish whatever you did before.

Attachment 202852 [details] pushed as ae088cb - Fix removal of libvirt machines