Looks good to me. See some comments below.
----- "David Huff" <dhuff@xxxxxxxxxx> wrote:
> This patch will run pre and post scripts
> defined in config file with the parameter pre_command
> and post_command post_command.
>
> Also exports all the prameters in preprocess for passing
> arguments to the script.
Why not do this for post_command as well?
> ---
> client/tests/kvm_runtest_2/kvm_preprocessing.py | 31
> +++++++++++++++++++++-
> 1 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/client/tests/kvm_runtest_2/kvm_preprocessing.py
> b/client/tests/kvm_runtest_2/kvm_preprocessing.py
> index c9eb35d..02df615 100644
> --- a/client/tests/kvm_runtest_2/kvm_preprocessing.py
> +++ b/client/tests/kvm_runtest_2/kvm_preprocessing.py
> @@ -135,8 +135,7 @@ def postprocess_vm(test, params, env, name):
> "Waiting for VM to kill itself..."):
> kvm_log.debug("'kill_vm' specified; killing VM...")
> vm.destroy(gracefully = params.get("kill_vm_gracefully") ==
> "yes")
> -
> -
> +
I hate to be petty, but we usually keep two blank lines between top
level functions.
Also, you have some trailing whitespace there...
> def process(test, params, env, image_func, vm_func):
> """Pre- or post-process VMs and images according to the
> instructions in params.
>
> @@ -169,6 +168,7 @@ def preprocess(test, params, env):
> params -- a dict containing all VM and image parameters
> env -- the environment (a dict-like object)
>
> + Also, runs any setup command defined in the parameter
> pre_command
> Also, collect some host information, such as the KVM version.
> """
> # Verify the identities of all living VMs
> @@ -192,6 +192,22 @@ def preprocess(test, params, env):
> vm.destroy()
> del env[key]
>
> + #execute any pre_commands
> + pre_command = params.get("pre_command")
> + if pre_command:
> + # export environment vars
> + for k in params.keys():
> + kvm_log.info("Adding KVM_TEST_%s to Environment" % (k))
> + os.putenv("KVM_TEST_%s" % (k), str(params[k]))
> +
> + # execute command
> + kvm_log.info("Executing command '%s'..." % pre_command)
> + (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> (test.bindir, pre_command),
> + None, kvm_log.debug,
> "(pre_command) ", timeout=600)
It wouldn't hurt to make this timeout user-configurable with a default
value of 600 or so:
timeout = int(params.get("pre_commmand_timeout", "600"))
(status, pid, output) = kvm_utils.run_bg(..., timeout=timeout)
We can also do that in a separate patch.
> + if status != 0:
> + kvm_utils.safe_kill(pid, signal.SIGTERM)
> + raise error.TestError, "Custom processing pre_command
> failed"
> +
> # Preprocess all VMs and images
> process(test, params, env, preprocess_image, preprocess_vm)
>
> @@ -232,6 +248,8 @@ def postprocess(test, params, env):
> test -- an Autotest test object
> params -- a dict containing all VM and image parameters
> env -- the environment (a dict-like object)
> +
> + Also, runs any command defined in the parameter post_command
> """
> process(test, params, env, postprocess_image, postprocess_vm)
>
> @@ -241,6 +259,15 @@ def postprocess(test, params, env):
> kvm_log.debug("'keep_ppm_files' not specified; removing all
> PPM files from results dir...")
> kvm_utils.run_bg("rm -vf %s" % os.path.join(test.debugdir,
> "*.ppm"), None, kvm_log.debug, "(rm) ", timeout=5.0)
>
> + #execute any post_commands
> + post_command = params.get("post_command")
> + if post_command:
> + kvm_log.info("Executing command '%s'..." % post_command)
> + (status, pid, output) = kvm_utils.run_bg("cd %s; %s" %
> (test.bindir, post_command),
> + None, kvm_log.debug,
> "(post_command) ", timeout=600)
> + if status != 0:
> + kvm_utils.safe_kill(pid, signal.SIGTERM)
> + raise error.TestError, "Custom processing command
> failed"
Same for post_command.
> def postprocess_on_error(test, params, env):
> """Perform postprocessing operations required only if the test
> failed.
Thanks,
Michael
--
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
