----- lookkas@xxxxxxxxx wrote: > Reviewers: lmr, > > Message: > 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") OK, no problem, but it might not be worth changing, because I've already sent the patch that gets rid of script_dir, image_dir and iso_dir. After applying that other patch, we can just change the config file to say: nic_script = scripts/qemu_ifup If you think it's important I'm fine with changing script_dir. > 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. OK, but I don't know how much we can explain in a comment. A wiki page should be set up that will do most of the explaining. > > 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. I had two reasons: 1. I wanted people to test TAP and complain if it doesn't work. 2. AFAIK, it is far more important to test TAP than user mode, so as soon as TAP is supported, it might seem reasonable to make it the default. On the other hand, TAP does require some manual setup and root privileges, so I can see why user mode can be a better default. I think the answer should come from the same people who made it clear to me that TAP is very important. Dor, Yaniv, what do you think? > 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. Jason Wang pointed out a problem with this approach: A guest with several NICs may receive an arp request on eth1, and reply via eth0, and that'll make verify_mac_ip_pair() fail, because it would seem that the tested IP address belongs to the wrong MAC address. I'm considering two solutions for this. Please see my reply to Jason Wang. > 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. Let's start with putting the script as a sample on the wiki. That's the easiest solution since we don't have to make any guarantees. If we conclude that a certain script will work for almost any host setup, then we can safely include that script under scripts/. > Description: > 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. > > Please review this at http://codereview.appspot.com/105083 > > Affected files: > M client/tests/kvm/control > A client/tests/kvm/kvm_address_pools.cfg.sample > A client/tests/kvm/kvm_cdkeys.cfg.sample > M client/tests/kvm/kvm_preprocessing.py > M client/tests/kvm/kvm_subprocess.py > M client/tests/kvm/kvm_tests.cfg.sample > M client/tests/kvm/kvm_tests.py > M client/tests/kvm/kvm_utils.py > M client/tests/kvm/kvm_vm.py > A client/tests/kvm/qemu-ifup I have to resolve the issue Jason Wang pointed out before I can send a fixed patch set. I'd rather not change the location of qemu-ifup right now because if I do I'll have to correct and resend the patch that gets rid of script_dir. It's such a minor issue that can be easily modified by the user, and on top of that, we're about to get rid of script_dir, assuming you agree with the other patch set. But again, as you wish -- let me know. Thank you very much for the thorough review. -- 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
