KVM autotest TAP patchset

Michael, the patchset looks mostly good to me, congrats!

I have a few comments to make. I believe it's enough to make you send me a full updated set with 9 patches:

1,2,3,4,5,8,9,11,12. 3 of them were already applied.

After this, I am going to apply it shortly.

[lmr, 2009-08-11 16:22:31]

Michael, the patchset looks mostly good to me, congrats!

I have a few comments to make. I believe it's enough to make you send me a full
updated set with 9 patches:

1,2,3,4,5,8,9,11,12. 3 of them were already applied.

After this, I am going to apply it shortly.

http://codereview.appspot.com/105083/diff/1/2
File client/tests/kvm/control (right):

http://codereview.appspot.com/105083/diff/1/2#newcode149
Line 149: cfg.parse_string("only ^default_host")
Ok, this solution is a lot less hassle than providing files with the names of
the hosts.

http://codereview.appspot.com/105083/diff/1/11
File client/tests/kvm/kvm_cdkeys.cfg.sample (right):

http://codereview.appspot.com/105083/diff/1/11#newcode4
Line 4: # You may uncomment them if necessary.
Ok, looks good to me.

http://codereview.appspot.com/105083/diff/1/4
File client/tests/kvm/kvm_preprocessing.py (right):

http://codereview.appspot.com/105083/diff/1/4#newcode49
Line 49: script_dir = test.bindir
At least before I check the patch that makes all paths specified relative to
test.bindir to simplify the code, I prefer the qemu-ifup script and script_dir
being defined as:

os.path.join(test.bindir, "scripts")

http://codereview.appspot.com/105083/diff/1/7
File client/tests/kvm/kvm_tests.cfg.sample (right):

http://codereview.appspot.com/105083/diff/1/7#newcode31
Line 31: address_index = 0
1) In the above block we could add a comment explaining what TAP and user modes
are, and point them to the documentation on how to set up the bridge.

2) I definitely think qemu-ifup and friends should go to scripts/ instead. We
could keep the config file definition right the way it is and change the
definition of scripts_dir as I suggested.

I was also wondering what was the reasoning for choosing TAP mode as the default
on the sample config file. User mode is more straightforward and doesn't need
any prior setup, so why make TAP default? Of course we can change it if we want
to test TAP provided it is well documented.

http://codereview.appspot.com/105083/diff/1/7#newcode120
Line 120: alive_test_cmd = uname -a
About ps vs uname: Since both are as good, uname -a is preferrable, as the
output is way shorter. For ps options, I myself allways prefer the form ps -ef
(GNU) in opposition to ps aux. In the end, all linux systems ship the GNU
variant anyway...

http://codereview.appspot.com/105083/diff/1/9
File client/tests/kvm/kvm_tests.py (right):

http://codereview.appspot.com/105083/diff/1/9#newcode572
Line 572: if se.get_command_status(params.get("alive_test_cmd")) != 0:
I see where the problem with DSL happened :) And yes, this way is much stricter,
and have good combination with uname -a, since that command wouldn't fail unless
there's something *really wrong* with the system, so this ends up failing only
for timeouts. good.

http://codereview.appspot.com/105083/diff/1/8
File client/tests/kvm/kvm_utils.py (right):

http://codereview.appspot.com/105083/diff/1/8#newcode167
Line 167: s.connect((ip, 55555))
Ok, It's not this weird, I see your point. By trying to connect to this ip on
this port you are updating the ARP cache with the actual MAC for this particular
IP so you can parse the ARP cache and get the actual MAC. Looks good to me, and
it seems to work on my test system.

http://codereview.appspot.com/105083/diff/1/3
File client/tests/kvm/kvm_vm.py (right):

http://codereview.appspot.com/105083/diff/1/3#newcode647
Line 647: "%s ---> %s" % (mac, ip))
I see your concern here about the verify_mac_ip_pair() check. Since we are
pickling the ip from the cache we got from tcpdump there might be mismatches.
However, I didn't see any on my tests here.

http://codereview.appspot.com/105083/diff/1/10
File client/tests/kvm/qemu-ifup (right):

http://codereview.appspot.com/105083/diff/1/10#newcode2
Line 2: 
As we already talked, this script as a sample looks and works OK when you
already have a bridge setup, we just need to add another auxiliary scripts that
can help the user to configure their bridges and either ship it under scripts/
or put them as a sample on the wiki documentation.