Here is the full text of the review http://codereview.appspot.com/183068/diff/1/3 File client/tests/kvm/tests/ksm_overcommit.py (right): http://codereview.appspot.com/183068/diff/1/3#newcode5 client/tests/kvm/tests/ksm_overcommit.py:5: import random, string, math, os Here we have the import of standard modules after the autotest and custom kvm ones. All standard module imports should be grouped together. http://codereview.appspot.com/183068/diff/1/3#newcode19 client/tests/kvm/tests/ksm_overcommit.py:19: def parse_meminfo(rowName): There are functions on client/bin/base_utils.py that already parse meminfo, we should consider using them instead of custom functions. http://codereview.appspot.com/183068/diff/1/3#newcode29 client/tests/kvm/tests/ksm_overcommit.py:29: Please don't use a single empty space between functions, as the autotest coding standard asks for 2: http://autotest.kernel.org/browser/trunk/CODING_STYLE I will omit the comment for further occurrences of single line spacing to avoid cluttering the review, but please fix them all. http://codereview.appspot.com/183068/diff/1/3#newcode42 client/tests/kvm/tests/ksm_overcommit.py:42: "VM: %s" % vm.name) Apparently a raise was forgotten on the line above. Also, the best way to do multi-line statements is using implicit continuation in parenthesis, in general you should refrain from using \ to continue sentences, ie: Instead of print "Fancy multi-line statement"+\ "that should be avoided." You should do print ("Fancy multi-line statement" "the preferred way.") I will omit the comment for further occurrences of ending lines with slash to avoid cluttering the review, but please fix them all. http://codereview.appspot.com/183068/diff/1/3#newcode88 client/tests/kvm/tests/ksm_overcommit.py:88: Mixing up the body of the main function with declaration of the auxiliary functions didn't work up well. Please make a clear separation between the body of the function run_ksm_overcommit and the declaration of auxiliary functions. http://codereview.appspot.com/183068/diff/1/3#newcode600 client/tests/kvm/tests/ksm_overcommit.py:600: Please don't leave up 4 lines of spacing on the code. http://codereview.appspot.com/183068/diff/1/2 File client/tests/kvm/unattended/allocator.py (right): http://codereview.appspot.com/183068/diff/1/2#newcode11 client/tests/kvm/unattended/allocator.py:11: 1) The above imports should be done according to the autotest coding standard, grouping them into a single line. 2) datetime.timedelta is not being used on the below code, as far as I can see 3) Instead of using datetime.now() use datetime.datetime.now(), I know it's not exactly pretty but it's the preferred way when writing autotest code. http://codereview.appspot.com/183068/diff/1/2#newcode13 client/tests/kvm/unattended/allocator.py:13: KVM test definitions. This summary of the program does not inform what the program is up for. I would suggest something along the lines "Auxiliary script used for memory allocation on guests" http://codereview.appspot.com/183068/diff/1/2#newcode16 client/tests/kvm/unattended/allocator.py:16: Jiri Zupka <jzupka@xxxxxxxxxx> Use @author, and your e-mail in parenthesis @author Jiri Zupka (jzupka@xxxxxxxxxx) http://codereview.appspot.com/183068/diff/1/2#newcode205 client/tests/kvm/unattended/allocator.py:205: print "PASS: Start" Why is that print statement for? It seems out of place On Mon, Dec 28, 2009 at 2:08 AM, <lookkas@xxxxxxxxx> wrote: > Hi Jiri, thank you very much for your work! I have comments to make > regarding to coding style as a first pass review. While reading them, > please keep in mind autotest's coding standards: > > http://autotest.kernel.org/browser/trunk/CODING_STYLE > > Also, I have noticed lots of trailing spaces on lines. I recommend that > every time you are done working with the code, you use the very useful > script utils/reindent.py (see top level autotest directory) that will > remove all trailing spaces on your code. While I wrote this first pass > review, I have applied it on the resultant files. > > > http://codereview.appspot.com/183068 > -- Lucas -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
