GNOME Bugzilla – Bug 681142
main: make --checks check for libvirt kvm
Last modified: 2016-03-31 13:53:43 UTC
Add check_libvirt_kvm () to check if we have the kvm capability on qemu:///session.
Created attachment 220248 [details] [review] main: make --checks check for libvirt kvm
Created attachment 220249 [details] [review] main: make --checks check for libvirt kvm Add check_libvirt_kvm () to check if we have the kvm capability on qemu:///session.
Created attachment 220255 [details] [review] main: make --checks check for libvirt kvm Add check_libvirt_kvm () to check if we have the kvm capability on qemu:///session.
Review of attachment 220255 [details] [review]: ::: src/util.vala @@ -446,2 +446,3 @@ } + public async bool check_libvirt_kvm () { We got APIs to be able to handle this all and since we use them in the same codebase, this code looks like a ugly hack.. :(
Review of attachment 220255 [details] [review]: ::: src/util.vala @@ -446,2 +446,3 @@ } + public async bool check_libvirt_kvm () { what API? Do we have a simple system libvirt check to see if kvm hvm is available for this user? Can we use it from --checks command line? What is really a hack when we ask the user on irc every time to dump the capabilities and checking the same result? Why not doing it once for all in --checks?
(In reply to comment #5) > Review of attachment 220255 [details] [review]: > > ::: src/util.vala > @@ -446,2 +446,3 @@ > } > > + public async bool check_libvirt_kvm () { > > what API? The libvirt capabilities API. Check vm-creator/configurator.vala for how to use that API. > Do we have a simple system libvirt check to see if kvm hvm is > available for this user? No, but thats what you are trying to implement, no? > Can we use it from --checks command line? Why not? You'll just have to create the default connection earlier I guess. > What is > really a hack when we ask the user on irc every time to dump the capabilities > and checking the same result? I doubt we'll have to rely on --checks that much anymore now that we do create VM even if KVM is unavailable and warn the user in the UI about the VM being slow. > Why not doing it once for all in --checks? Although I don't think this is very important anymore, I'm not against this change. I'm only objecting on your implementation. Its just not important enough anymore to justify a hackish implementation.
In case you still don't agree with my sense of priority on this, I don't think this is important enough to debate about so if you disagree to my previous comments, please feel free to push this.
(In reply to comment #6) > > Although I don't think this is very important anymore, I'm not against this > change. I'm only objecting on your implementation. Its just not important > enough anymore to justify a hackish implementation. The point is performing simple tests of the systems, not to test wether boxes/libvirt code works well with the system. The two are different. Imho, the more tests/information we have in --checks, the better. So it may look like a hack to you, but it looks better to me than asking the user to paste their virsh capabilities and reading a XML file manually
(In reply to comment #8) > (In reply to comment #6) > > > > Although I don't think this is very important anymore, I'm not against this > > change. I'm only objecting on your implementation. Its just not important > > enough anymore to justify a hackish implementation. > > The point is performing simple tests of the systems, not to test wether > boxes/libvirt code works well with the system. The two are different. > > Imho, the more tests/information we have in --checks, the better. > > So it may look like a hack to you, but it looks better to me than asking the > user to paste their virsh capabilities and reading a XML file manually Once again, I have nothing against having this check. Please read my comments carefully before replying further. As I said, if you think this is not a hack after my explanation, go ahead and commit it. However, would still be nice to know why you insist that this code can't be implemented using the APIs.
(In reply to comment #9) > Once again, I have nothing against having this check. Please read my comments > carefully before replying further. As I said, if you think this is not a hack > after my explanation, go ahead and commit it. However, would still be nice to > know why you insist that this code can't be implemented using the APIs. Precisely because I want as low as possible system checks, not something higher level like using libvirt directly, or libvirt-glib or higher in boxes code. We need low-level checks. One we should add too is some kind of file system check and disk space. There is probably more we can add, such as free memory etc..
(In reply to comment #10) > (In reply to comment #9) > > Once again, I have nothing against having this check. Please read my comments > > carefully before replying further. As I said, if you think this is not a hack > > after my explanation, go ahead and commit it. However, would still be nice to > > know why you insist that this code can't be implemented using the APIs. > > Precisely because I want as low as possible system checks, not something higher > level like using libvirt directly, or libvirt-glib or higher in boxes code. In this case, the code is not that much different than what you'd get through libvirt-glib. virsh will call the same libvirt function as libvirt-glib to get the capability XML, and then your code and libvirt-glib will use libxml2 to parse it. I understand your approach, but in this particular case, it's unlikely that doing the parsing ourselves or using libvirt-glib will make a difference.
Attachment 220255 [details] pushed as ffecefd - main: make --checks check for libvirt kvm
(In reply to comment #11) > In this case, the code is not that much different than what you'd get through > libvirt-glib. virsh will call the same libvirt function as libvirt-glib to get > the capability XML, and then your code and libvirt-glib will use libxml2 to > parse it. I understand your approach, but in this particular case, it's > unlikely that doing the parsing ourselves or using libvirt-glib will make a > difference. Except that we don't rely on libvirt-glib this way, and make sure virsh works too. This is enough difference to me to justify the check this way. The reality is that we rely on other programs to do the sanity checks when helping users.
(In reply to comment #13) >The reality is that we rely on other programs to do the sanity checks when helping users. Well, of course, it's not really practical to call library functions from a terminal...