Created attachment 208619 [details] [review]
Make use of Libvirt.Domain.get_devices()

Created attachment 208714 [details] [review]
Make use of Libvirt.Domain.get_devices()

Same patch, only that it ensures we are getting statistics for the correct disk.

Review of attachment 208714 [details] [review]:

not really interested by this change at this point (it was working fine), but since you did it

::: src/libvirt-machine.vala
@@ +147,1 @@
             // FIXME: switch to domain.get_devices () and loop over all interfaces

ok to keep the FIXME, because we aren't yet stats'ing all the interfaces, but you could perhaps update it.

but why do you remove it for the other? it is a marker everywhere a related change is needed.

@@ +152,3 @@
+                    if (disk_type == GVirConfig.DomainDiskGuestDeviceType.DISK)
+                        break;
+                    else

the else isn't really necessary here

(In reply to comment #3)
> Review of attachment 208714 [details] [review]:
> 
> not really interested by this change at this point (it was working fine), but
> since you did it

I mainly did this patch to test the new API/changes and because I'll be using this in bug#664774 in the same module.

> ::: src/libvirt-machine.vala
> @@ +147,1 @@
>              // FIXME: switch to domain.get_devices () and loop over all
> interfaces
> 
> ok to keep the FIXME, because we aren't yet stats'ing all the interfaces, but
> you could perhaps update it.

K

> but why do you remove it for the other? it is a marker everywhere a related
> change is needed.

You meant to write "why don't you"?

> @@ +152,3 @@
> +                    if (disk_type ==
> GVirConfig.DomainDiskGuestDeviceType.DISK)
> +                        break;
> +                    else
> 
> the else isn't really necessary here

the 'disk' variable is already set so it needs to be unset. Now that I think of it, its only needed on the last loop.

Created attachment 208733 [details] [review]
Make use of Libvirt.Domain.get_devices()

This version attempts to address the issues pointed out in previous review and adds warning for the related errors.

Created attachment 208819 [details] [review]
Make use of Libvirt.Domain.get_devices()

Refactored version that prefers Boxes' managed volume over other disks.

Created attachment 208832 [details] [review]
Make use of Libvirt.Domain.get_devices()

Same but with some minor coding-style fixes.

(In reply to comment #6)
> Created an attachment (id=208819) [details] [review]
> Make use of Libvirt.Domain.get_devices()
> 
> Refactored version that prefers Boxes' managed volume over other disks.

BTW, this is something the existing code doesn't do at all and since it shows statistics for the first disk in the config, there is no guarantees that the stats we get are for the main volume and not the floppy or cdrom disk..

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=208819) [details] [review] [details] [review]
> > Make use of Libvirt.Domain.get_devices()
> > 
> > Refactored version that prefers Boxes' managed volume over other disks.
> 
> BTW, this is something the existing code doesn't do at all and since it shows
> statistics for the first disk in the config, there is no guarantees that the
> stats we get are for the main volume and not the floppy or cdrom disk..

Doing it over only the disk created by boxes is as bad, as it may not even work with VM created with say virt-manager, and we should be fully compatible with those.

Also, it should stats over all IO: all disks, all interfaces, ..

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > Created an attachment (id=208819) [details] [review] [details] [review] [details] [review]
> > > Make use of Libvirt.Domain.get_devices()
> > > 
> > > Refactored version that prefers Boxes' managed volume over other disks.
> > 
> > BTW, this is something the existing code doesn't do at all and since it shows
> > statistics for the first disk in the config, there is no guarantees that the
> > stats we get are for the main volume and not the floppy or cdrom disk..
> 
> Doing it over only the disk created by boxes is as bad, as it may not even work
> with VM created with say virt-manager, and we should be fully compatible with
> those.

Its not doing that. It only uses the boxes' created volume if there is one. I made sure of this.

> Also, it should stats over all IO: all disks, all interfaces, ..

Yeah, that could be another patch.

Oh and I forgot to mention that I made sure to break the existing system totally against latest git master of libvirt-glib. :P

(In reply to comment #10)

> Its not doing that. It only uses the boxes' created volume if there is one. I
> made sure of this.

Can you make sure it also works with VM created with virt-manager? It used to work.

Review of attachment 208832 [details] [review]:

it seems to work as well as before with virt-manager vm, so ack, since I can't spot regressions.

Attachment 208832 [details] pushed as 02040fc - Make use of Libvirt.Domain.get_devices()