On Tue, 2011-07-05 at 07:51 +0530, Pradeep Kumar wrote:
> From 2813840a93e9679f224cf6c78c83b340c1d46964 Mon Sep 17 00:00:00 2001
> From: pradeepkumar <sp@pradeep.(none)>
> Date: Tue, 5 Jul 2011 07:08:05 +0530
> Subject: [PATCH] [AUTOTEST][KVM] verifying smbios table for guest patch
> Signed-off-by: pradeepkumar <[email protected]>
Thanks for fixing this! Some comments below, please let me know if I am
not clear enough with my reasoning.
>
> new file: client/tests/kvm/tests/smbios_table.py
> modified: client/tests/kvm/tests_base.cfg.sample
> ---
> client/tests/kvm/tests/smbios_table.py | 62
> ++++++++++++++++++++++++++++++++
> client/tests/kvm/tests_base.cfg.sample | 3 ++
> 2 files changed, 65 insertions(+), 0 deletions(-)
> create mode 100644 client/tests/kvm/tests/smbios_table.py
>
> diff --git a/client/tests/kvm/tests/smbios_table.py
> b/client/tests/kvm/tests/smbios_table.py
> new file mode 100644
> index 0000000..5b11441
> --- /dev/null
> +++ b/client/tests/kvm/tests/smbios_table.py
> @@ -0,0 +1,62 @@
> +import commands, logging
> +from autotest_lib.client.common_lib import error
> +from autotest_lib.client.virt import virt_env_process, virt_test_utils
> +
> +def run_smbios_table(test, params, env):
^ above that line you can put the @error.context_aware decorator. Please
refer to the unattended_install test for an example of how it's used.
> + """
> + Check Memory ballooning:
^ This part of the docstring doesn't seem to belong here.
> + 1) Boot a guest with smbios options
> + 2) verify if host bios options have been emulated
> +
> + @param test: kvm test object
> + @param params: Dictionary with the test parameters
> + @param env: Dictionary with test environment.
> + """
> +
> + s_vendor, vendor = commands.getstatusoutput("dmidecode --type 0 | grep
> Vendor | awk '{print $2}'")
> + if s_vendor != 0:
> + raise error.TestFail("couldnt get Vendor")
^ I'd prefer you use utils.system_output(), because that function will
throw an error.CmdError anyway, so we can simplify the code quite a bit.
It'd look like:
error.context("getting smbios table")
vendor = utils.system_output("dmidecode --type 0 | grep Vendor | awk '{print
$2}'")
Don't forget to add the decorator on the place I have oriented.
> +
> + s_date, date = commands.getstatusoutput("dmidecode --type 0 | grep Date
> | awk '{print $3}'")
> + if s_date != 0:
> + raise error.TestFail("couldnt get Date")
> +
> + s_version, version = commands.getstatusoutput("dmidecode --type 0 | grep
> Version | awk '{print $2}'")
> + if s_version != 0:
> + raise error.TestFail("couldnt get version")
> +
> + def boot_with_smbios():
^ Be safe and make this function take params as a parameter. Or better,
rather than having this function you can just move all the instructions
in this function to the toplevel function, factoring this out as a
function is unnecessary.
> + """
> + boot geust with host bios options
> + """
> + params['extra_params'] = standard_extra_params
^ Inconsistent indentation. I suggest you to use the script
utils/reindent.py always before you finish your patches, as it'll
automatically fix all indentation issues. You can also run
utils/run_pylint.py to avoid common syntax mistakes.
> + params['extra_params'] += ("-smbios
> type=0,vendor=%s,version=%s,date=%s" % (vendor, version, date))
> +
> + logging.debug("Booting guest %s", params.get("main_vm"))
> + virt_env_process.preprocess_vm(test, params, env,
> params.get("main_vm"))
> +
> + vm = env.get_vm(params["main_vm"])
> + login_timeout = float(params.get("login_timeout", 360))
> + session = virt_test_utils.wait_for_login(vm, timeout=login_timeout)
> +
> + guest_vendor = session.cmd("dmidecode --type 0 | grep Vendor | awk
> '{print $2}'")
> + guest_date = session.cmd("dmidecode --type 0 | grep Date | awk
> '{print $3}'")
> + guest_version = session.cmd("dmidecode --type 0 | grep Version | awk
> '{print $2}'")
You can avoid to repeat commands by writing a function that:
a) Executes command on host and stores result
b) Executes command on guest, stores result
c) compares them, and returns pass/fail
Then you can go through this list of 3 commands. It's important to not
fail the test away on just one failure, go through the 3 commands and
only fail the test after we went through all of them. It'll fix the last
remark
>
> + if vendor != guest_vendor.strip():
> + logging.info("Vendor is not matching")
> +
> + if date != guest_date.strip():
> + logging.info("Date is not matching")
> +
> + if version != guest_version.strip():
> + logging.info("Version is not matching")
> +
> +
> + # INITIALIZE
> + if "extra_params" in params:
> + standard_extra_params = params['extra_params']
> + else:
> + standard_extra_params = ""
> + boot_with_smbios()
^ There's no criteria of pass/fail here. So, we need to keep track of
whether the comparison 'dmidecode field on host' == 'dmidecode field on
guest' yields false, and if it happens to any of the fields we're
interested in, then FAIL the test. If all comparisons yielded true, then
the test PASSed.
> diff --git a/client/tests/kvm/tests_base.cfg.sample
> b/client/tests/kvm/tests_base.cfg.sample
> index 1a86265..e02d536 100644
> --- a/client/tests/kvm/tests_base.cfg.sample
> +++ b/client/tests/kvm/tests_base.cfg.sample
> @@ -416,6 +416,9 @@ variants:
> extra_params += " -watchdog i6300esb -watchdog-action reset"
> relogin_timeout = 240
>
> + - smbios_table: install setup image_copy unattended_install.cdrom
> + type = smbios_table
^ Since we are going to start the vm with preprocess_vm, I believe you
should put start_vm = no here.
> +
> - stress_boot: install setup image_copy unattended_install.cdrom
> type = stress_boot
> max_vms = 5
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
