GNOME Bugzilla – Bug 671673
add --checks option to troubleshot for vt/kvm
Last modified: 2016-03-31 13:57:55 UTC
Add a simple argument --check to do some basic capability checking. Although we are in freeze time, it can be helpful to help people who expect boxes to work on their machine explain why it doesn't. In a future version of boxes, we should have a translatable and friendlier version of this warning, with a documentation explaining how to enable VT in the bios and to load KVM.
Created attachment 209275 [details] [review] add --checks option to troubleshot for vt/kvm
Review of attachment 209275 [details] [review]: First question that comes to mind: Why not make use of libvirt's capabilities API?
(In reply to comment #2) > First question that comes to mind: Why not make use of libvirt's capabilities > API? Because libvirt will not be able to tell you why KVM is not available. it will just tell you it is not. The idea here is to help the user: 1. is your hardware capable, or not 2. do you have the correct module loaded With this we can provide appropriate error messages and help pages to guide users. The libvirt capabilities aren't really designed for this afaik.
(In reply to comment #3) > (In reply to comment #2) > > First question that comes to mind: Why not make use of libvirt's capabilities > > API? > > Because libvirt will not be able to tell you why KVM is not available. it will > just tell you it is not. > > The idea here is to help the user: > 1. is your hardware capable, or not > 2. do you have the correct module loaded Hmm.. maybe its good for now but we should fix this in libvirt? I would have insisted on getting this in libvirt from start but knowing how over-obsessed libvirt devs are about portability, I don't think this will get in there anytime soon.
(In reply to comment #4) > > The idea here is to help the user: > > 1. is your hardware capable, or not > > 2. do you have the correct module loaded > > Hmm.. maybe its good for now but we should fix this in libvirt? I would have > insisted on getting this in libvirt from start but knowing how over-obsessed > libvirt devs are about portability, I don't think this will get in there > anytime soon. I understand your concern, but I think you misunderstand the goal of libvirt. It is not to help an end-user figure out why he can't use kvm, but just be a solid abstraction for various virtualization technologies. Since Boxes is currently tight to kvm, and I don't think that will change soon, and we are targetting end users, we need to help them figure out this issues and help them solve it. Currently, the way you do it either with kvm or libvirt is go read a wiki. I want to avoid this, and I also want to avoid saying to a user to go to a console and type grep /proc/cpuinfo | grep -E (vmx|..) etc.. The basics tests we need to perform (assuming the distrubution did the rest of the job correctly) are: hardware capabilities (you can't do VM) and kvm is loaded (you might need to turn on Bios option, reboot or complain to your distro) That's why I think we need those two basic tests. (we might want to add more in the future, like depending on some libvirt/qemu capabilities)
Review of attachment 209275 [details] [review]: ::: src/util.vala @@ +436,3 @@ + } + + (I am missing a space here)
Review of attachment 209275 [details] [review]: ::: src/main.vala @@ -8,2 +9,4 @@ private const OptionEntry[] options = { { "version", 0, 0, OptionArg.NONE, ref version, N_("Display version number"), null }, + // FIXME: make it translatable after string freeze + { "checks", 0, 0, OptionArg.NONE, ref checks, "Check virtualization capabilities", null }, Do we really want an option for this if all we are doing is to print some messages on the console? I think we should do this always. @@ -38,2 +41,5 @@ exit (0); } + + if (checks) { + var loop = new GLib.MainLoop (); is the GLib prefix really needed here (and in other places in this file)? @@ -40,0 +43,13 @@ + + if (checks) { + var loop = new GLib.MainLoop (); ... 10 more ... visibility modifier missing here too.. @@ +54,3 @@ + +async void run_checks () { + var cpu = yield Boxes.cpu_can_virtualize (); Do we need 'Boxes' namespace here? ::: src/util.vala @@ -395,2 +395,4 @@ public abstract string XSession { owned get; } } + + string yes_no (bool value) { * better name IMO: bool_to_string * don't forget the visibility modifier. @@ -397,0 +397,6 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 3 more ... Better (IMO) name suggestion: check_cpu_can_virtualize() @@ -397,0 +397,11 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 8 more ... From CodingStyle.txt: * ''Prefer'' descriptive names over abbreviations (unless well-known) & shortening of names. E.g discoverer over disco. @@ -397,0 +397,18 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 15 more ... same here: 'e' -> 'error'. Yes, I know it will then conflict with the 'error' call below but thats where use of namespaces comes handy: GLib.error (error.message); @@ +413,3 @@ + } + } catch (GLib.Error e) { + error (e.message); Unless you are certain that e.message here will be helpful, I think appending it with some context is highly recommended. @@ -397,0 +397,22 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 19 more ... 'what' can virtualize? How about "Hardware Virtualization extensions present: " ? @@ -397,0 +397,26 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 23 more ... Better (IMO) name: check_kvm_module_loaded() @@ -397,0 +397,42 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 39 more ... Same here. How about "Kernel-based Virtual Machine (KVM) module loaded: " + NAME_OF_MODULE/None?
(In reply to comment #5) > (In reply to comment #4) > > > The idea here is to help the user: > > > 1. is your hardware capable, or not > > > 2. do you have the correct module loaded > > > > Hmm.. maybe its good for now but we should fix this in libvirt? I would have > > insisted on getting this in libvirt from start but knowing how over-obsessed > > libvirt devs are about portability, I don't think this will get in there > > anytime soon. > > I understand your concern, but I think you misunderstand the goal of libvirt. > It is not to help an end-user figure out why he can't use kvm, but just be a > solid abstraction for various virtualization technologies. No, I didn't say that libvirt should do all the work including interaction with the user. :) I was only saying that libvirt should provide APIs to check these things and then Boxes should use those APIs instead of poking directly into /proc/ etc. That way other apps can do the same (which is why we make max use of libvirt). > That's why I think we need those two basic tests. (we might want to add more in > the future, like depending on some libvirt/qemu capabilities) Sure, I'm not at all against this patch's goals. There is a reason why I just did a very detailed review of it. :)
Review of attachment 209275 [details] [review]: ::: src/main.vala @@ -8,2 +9,4 @@ private const OptionEntry[] options = { { "version", 0, 0, OptionArg.NONE, ref version, N_("Display version number"), null }, + // FIXME: make it translatable after string freeze + { "checks", 0, 0, OptionArg.NONE, ref checks, "Check virtualization capabilities", null }, if do check it always, but it's not printed by default, only if you enable G_MESSAGES.. I wanted a console report functionnality too, without launching the UI. @@ -38,2 +41,5 @@ exit (0); } + + if (checks) { + var loop = new GLib.MainLoop (); yes, we don't need to debate this again. @@ -40,0 +43,13 @@ + + if (checks) { + var loop = new GLib.MainLoop (); ... 10 more ... ack ::: src/util.vala @@ -395,2 +395,4 @@ public abstract string XSession { owned get; } } + + string yes_no (bool value) { * bool.to_string() already exists, and is not "yes" "no", it is "true" "false". * ack for visibility change @@ -397,0 +397,6 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 3 more ... "check" and "can" kinda redundant, but ok. @@ -397,0 +397,11 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 8 more ... dis = DataInputStream.. I mean it's very often that we use initials for short-live variables., like "i" for iterator... do you really prefer var data_input_stream = ...? @@ -397,0 +397,18 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 15 more ... same reasons as above why we use "i" but ok.. ack @@ +413,3 @@ + } + } catch (GLib.Error e) { + ack @@ -397,0 +397,22 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 19 more ... this is only debugging message, but ack @@ -397,0 +397,26 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 23 more ... ack @@ -397,0 +397,42 @@ + + string yes_no (bool value) { + return value ? "yes" : "no"; ... 39 more ... again, this is not for endusers, but merely for us, developers.
Created attachment 209292 [details] [review] add --checks option to troubleshot for vt/kvm Add a simple argument --check to do some basic capability checking. Although we are in freeze time, it can be helpful to help people who expect boxes to work on their machine explain why it doesn't. In a future version of boxes, we should have a translatable and friendlier version of this warning, with a documentation explaining how to enable VT in the bios and to load KVM.
(In reply to comment #9) > Review of attachment 209275 [details] [review]: > > ::: src/main.vala > @@ -8,2 +9,4 @@ > private const OptionEntry[] options = { > { "version", 0, 0, OptionArg.NONE, ref version, N_("Display version > number"), null }, > + // FIXME: make it translatable after string freeze > + { "checks", 0, 0, OptionArg.NONE, ref checks, "Check virtualization > capabilities", null }, > > if do check it always, but it's not printed by default, only if you enable > G_MESSAGES.. I wanted a console report functionnality too, without launching > the UI. We can just make it so that we always print this on the console through 'print' or 'stdout.printf'. I don't think anyone would mind if we always provide this information. > @@ -38,2 +41,5 @@ > exit (0); > } > + > + if (checks) { > + var loop = new GLib.MainLoop (); > > yes, we don't need to debate this again. Hm.. I thought we agreed that we use prefix when there is ambiguity of any kind but not a big deal.. > ::: src/util.vala > @@ -395,2 +395,4 @@ > public abstract string XSession { owned get; } > } > + > + string yes_no (bool value) { > > * bool.to_string() already exists, and is not "yes" "no", it is "true" "false". Good to know but 'bool_to_string' is still better than 'yes_no'? :) > @@ -397,0 +397,11 @@ > + > + string yes_no (bool value) { > + return value ? "yes" : "no"; > ... 8 more ... > > dis = DataInputStream.. I mean it's very often that we use initials for > short-live variables., like "i" for iterator... > > do you really prefer var data_input_stream = ...? No but is 'stream' already used in that context? If it is, then 'data_stream' or 'input_stream' aren't bad names either. > @@ -397,0 +397,18 @@ > + > + string yes_no (bool value) { > + return value ? "yes" : "no"; > ... 15 more ... > > same reasons as above why we use "i" but ok.. ack Splinter isn't doing a good job here so I don't know what this is in reply to. :( As for use 'i' as iterator variable, thats a very common practice in many languages so its obvious to most programmers. > @@ -397,0 +397,22 @@ > + > + string yes_no (bool value) { > + return value ? "yes" : "no"; > ... 19 more ... > > this is only debugging message, but ack Yeah, I know i'm being too strict here but I wouldn't mind being a bit more friendly to developers as well. :)
(In reply to comment #11) > > We can just make it so that we always print this on the console through 'print' > or 'stdout.printf'. I don't think anyone would mind if we always provide this > information. I personally prefer UI application not to print anything useful on the console by default. We should do the check at runtime with UI feedback. But since we are in translatable string freeze, I have no choice but to just add a command line option. The idea is to do provide checks in the future, so there will be more output. And --checks allows to do that in the console without running the UI! It's the console approach vs the future integration in the UI (disabling functionnalities, providing dialogs and help pages etc..) > > * bool.to_string() already exists, and is not "yes" "no", it is "true" "false". > > Good to know but 'bool_to_string' is still better than 'yes_no'? :) bool.to_string() -> true/false bool_to_string () -> yes/no That would be terribly confusing. Let's keep yes_no() > > > @@ -397,0 +397,11 @@ > > + > > + string yes_no (bool value) { > > + return value ? "yes" : "no"; > > ... 8 more ... > > > > dis = DataInputStream.. I mean it's very often that we use initials for > > short-live variables., like "i" for iterator... > > > > do you really prefer var data_input_stream = ...? > > No but is 'stream' already used in that context? If it is, then 'data_stream' > or 'input_stream' aren't bad names either. A stream and an input_stream are different kind of gio objects, that is more confusing. > > @@ -397,0 +397,18 @@ > > + > > + string yes_no (bool value) { > > + return value ? "yes" : "no"; > > ... 15 more ... > > > > same reasons as above why we use "i" but ok.. ack > > Splinter isn't doing a good job here so I don't know what this is in reply to. > :( As for use 'i' as iterator variable, thats a very common practice in many > languages so its obvious to most programmers. So is "e" for catching errors in Vala (and I guess in C# or Java too) > Yeah, I know i'm being too strict here but I wouldn't mind being a bit more > friendly to developers as well. :) friendly for developpers is "grepable", and I just made it more grepable by using function name.
(In reply to comment #12) > (In reply to comment #11) > > > > We can just make it so that we always print this on the console through 'print' > > or 'stdout.printf'. I don't think anyone would mind if we always provide this > > information. > > I personally prefer UI application not to print anything useful on the console > by default. > > We should do the check at runtime with UI feedback. But since we are in > translatable string freeze, I have no choice but to just add a command line > option. > > The idea is to do provide checks in the future, so there will be more output. > And --checks allows to do that in the console without running the UI! It's the > console approach vs the future integration in the UI (disabling > functionnalities, providing dialogs and help pages etc..) Ah I missed the fact that UI is not run when use these options. > > > > > @@ -397,0 +397,11 @@ > > > + > > > + string yes_no (bool value) { > > > + return value ? "yes" : "no"; > > > ... 8 more ... > > > > > > dis = DataInputStream.. I mean it's very often that we use initials for > > > short-live variables., like "i" for iterator... > > > > > > do you really prefer var data_input_stream = ...? > > > > No but is 'stream' already used in that context? If it is, then 'data_stream' > > or 'input_stream' aren't bad names either. > > A stream and an input_stream are different kind of gio objects, that is more > confusing. The name doesn't imply its type and besides a DataInputStream *is* both a Stream and InputStream. > > > @@ -397,0 +397,18 @@ > > > + > > > + string yes_no (bool value) { > > > + return value ? "yes" : "no"; > > > ... 15 more ... > > > > > > same reasons as above why we use "i" but ok.. ack > > > > Splinter isn't doing a good job here so I don't know what this is in reply to. > > :( As for use 'i' as iterator variable, thats a very common practice in many > > languages so its obvious to most programmers. > > So is "e" for catching errors in Vala (and I guess in C# or Java too) Oh? I learnt something new today so I'm happy but also sad that I changed this in many places in Boxes already long time ago. :( > > Yeah, I know i'm being too strict here but I wouldn't mind being a bit more > > friendly to developers as well. :) > > friendly for developpers is "grepable", and I just made it more grepable by > using function name. How is it less greppable? `grep "whatever was printed or part of it even" src/*.vala'` always does the trick. Anyway, this is also more like a nitpick so no biggie if you ignore this..
(In reply to comment #13) > > A stream and an input_stream are different kind of gio objects, that is more > > confusing. > > The name doesn't imply its type and besides a DataInputStream *is* both a > Stream and InputStream. It's good when it does, especially when the variable/object doesn't have a strong meaning but is rather just an intermadiate object/interface. And the method used (read_line) is only on DataInputStream, so it's nice to keep this relation. (DataInputStream is an InputStream, but not a Stream as such type doesn't exist) In short, I prefer: var dis = new DataInputStream(foo) // common idiom using type name for short-live variables, variable name matches type while (line = yield dis.read_line ()) while you prefer: var stream = new DataInputStream(foo) // nice (but generic == can clash), no direct type relation while (line = yield stream.read_line()) Ok, I'd say you are right in this case, but we shouldn't this kind of naming imho, since I also had good reasons to name it "dis", and I hope you agreed.
I'd say you are right in this case, but we shouldn't debate this kind of naming imho, since I also had good reasons to name it "dis", and I hope you can agree.
Created attachment 209296 [details] [review] add --checks option to troubleshot for vt/kvm Add a simple argument --check to do some basic capability checking. Although we are in freeze time, it can be helpful to help people who expect boxes to work on their machine explain why it doesn't. In a future version of boxes, we should have a translatable and friendlier version of this warning, with a documentation explaining how to enable VT in the bios and to load KVM.
Review of attachment 209296 [details] [review]: Assuming you tested this, ACK!
fixed in git
Just my 2 cents, "stream" is far more readable to the casual reader (for example if the name appears in a backtrace, or to someone discovering the code for the first time and trying to work on a simple bug fix). With "stream", it's obvious that the variable is some kind of stream, while "dis" doesn't ring any bell (it's not even guaranteed that someone will understand what "dis" stands even if he knows it's a DataInputStream) and any time this name will be encountered, some extra mental effort will be necessary to do the translation, which is not needed at all if the variable is name "stream", and should not be that confusing unless both GStream and GDataInputStream are used in the same function. For similar reasons, when using global functions/static methods/..., prefixing the function name with the namespace (except maybe for the Boxes namespace) where they come from makes things much easier for someone not fully into the project. You don't have to make guesses to try and find where a given function comes from.
(In reply to comment #19) > For similar reasons, when using global functions/static methods/..., prefixing > the function name with the namespace (except maybe for the Boxes namespace) > where they come from makes things much easier for someone not fully into the > project. You don't have to make guesses to try and find where a given function > comes from. FWIW, the namespace in question here is 'GLib', which is treated specially in Vala when using glib profile in the sense that its automatically imported for you (i-e 'using GLib' is implied). Having said that, I can agree to the rule of always using the namespace but if someone could volunteer to update the CodingStyle.txt and update the code to make this a consistent practice. IMO inconsistency annoys/confuses the reader more than anything else.