Hi Amos, thanks for the patch, here are my comments (pretty much concerning only coding style): On Wed, Sep 23, 2009 at 8:19 AM, Amos Kong <akong@xxxxxxxxxx> wrote: > Test 802.1Q vlan of nic, config it by vconfig command. > 1) Create two VMs > 2) Setup guests in different vlan by vconfig and test communication by ping > using hard-coded ip address > 3) Setup guests in same vlan and test communication by ping > 4) Recover the vlan config > > Signed-off-by: Amos Kong <akong@xxxxxxxxxx> > --- > client/tests/kvm/kvm_tests.cfg.sample | 6 +++ > client/tests/kvm/tests/vlan_tag.py | 66 +++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+), 0 deletions(-) > create mode 100644 client/tests/kvm/tests/vlan_tag.py > > diff --git a/client/tests/kvm/kvm_tests.cfg.sample b/client/tests/kvm/kvm_tests.cfg.sample > index 285a38f..5a3f97d 100644 > --- a/client/tests/kvm/kvm_tests.cfg.sample > +++ b/client/tests/kvm/kvm_tests.cfg.sample > @@ -145,6 +145,12 @@ variants: > kill_vm = yes > kill_vm_gracefully = no > > + - vlan_tag: install setup > + type = vlan_tag > + subnet2 = 192.168.123 > + vlans = "10 20" > + nic_mode = tap > + nic_model = e1000 > > # NICs > variants: > diff --git a/client/tests/kvm/tests/vlan_tag.py b/client/tests/kvm/tests/vlan_tag.py > new file mode 100644 > index 0000000..2904276 > --- /dev/null > +++ b/client/tests/kvm/tests/vlan_tag.py > @@ -0,0 +1,66 @@ > +import logging, time > +from autotest_lib.client.common_lib import error > +import kvm_subprocess, kvm_test_utils, kvm_utils > + > +def run_vlan_tag(test, params, env): > + """ > + Test 802.1Q vlan of nic, config it by vconfig command. > + > + 1) Create two VMs > + 2) Setup guests in different vlan by vconfig and test communication by ping > + using hard-coded ip address > + 3) Setup guests in same vlan and test communication by ping > + 4) Recover the vlan config > + > + @param test: Kvm test object > + @param params: Dictionary with the test parameters. > + @param env: Dictionary with test environment. > + """ > + > + vm = [] > + session = [] > + subnet2 = params.get("subnet2") > + vlans = params.get("vlans").split() > + > + vm.append(kvm_test_utils.get_living_vm(env, "%s" % params.get("main_vm"))) > + > + params_vm2 = params.copy() > + params_vm2['image_snapshot'] = "yes" > + params_vm2['kill_vm_gracefully'] = "no" > + params_vm2["address_index"] = int(params.get("address_index", 0))+1 > + vm.append(vm[0].clone("vm2", params_vm2)) > + kvm_utils.env_register_vm(env, "vm2", vm[1]) > + if not vm[1].create(): > + raise error.TestError, "VM 'vm[1]' create faild" In the above exception raise statement, the preferred form to do it is: raise error.TestError("VM 1 create failed") > + for i in range(2): > + session.append(kvm_test_utils.wait_for_login(vm[i])) > + > + try: > + vconfig_cmd = "vconfig add eth0 %s;ifconfig eth0.%s %s.%s" > + if session[0].get_command_status(vconfig_cmd % (vlans[0], > + vlans[0], > + subnet2, > + "11")) != 0 or \ > + session[1].get_command_status(vconfig_cmd % (vlans[1], > + vlans[1], > + subnet2, > + "12")) != 0: In the above if statement, I'd assign the comparisons to variables to make the code more readable, like: try: vconfig_cmd = "vconfig add eth0 %s;ifconfig eth0.%s %s.%s" # Attempt to configure IPs for the VMs and record the results in boolean # variables ip_config_vm1_ok = (session[0].get_command_status( vconfig_cmd % (vlans[0], vlans[0], subnet2, "11")) == 0) ip_config_vm1_ok = (session[1].get_command_status( vconfig_cmd % (vlans[1], vlans[1], subnet2, "12")) == 0) if not ip_config_vm1_ok or not ip_config_vm2_ok: > + raise error.TestError, "Fail to config VMs ip address" > + if session[0].get_command_status("ping -c 2 %s.12" % subnet2) == 0: > + raise error.TestFail("Guest is unexpectedly pingable in different " > + "vlan") A similar comment applies to the above block > + if session[1].get_command_status("vconfig rem eth0.%s;vconfig add eth0 " > + "%s;ifconfig eth0.%s %s.12" % > + (vlans[1], > + vlans[0], > + vlans[0], > + subnet2)) != 0: Idem > + raise error.TestError, "Fail to config ip address of VM 'vm[1]'" > + if session[0].get_command_status("ping -c 2 %s.12" % subnet2) != 0: > + raise error.TestFail, "Fail to ping the guest in same vlan" See comment about the preferred form to write raise statements. > + finally: > + for i in range(2): > + session[i].sendline("vconfig rem eth0.%s" % vlans[0]) > + session[i].close() > -- > 1.5.5.6 Please send me an updated version of the patch. 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