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 <ak...@redhat.com> 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 <ak...@redhat.com>
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
