This is a good start, and I agree we need more flexibility here. More
feedback inline...
On 06/01/2012 02:39 AM, guyanhua wrote:
>
> Adding two params (default raise_error = True, extra_param = "")in
> virsh_cmd function,
> which can satisfy the basic use and test the virsh command through diff
> params.
> Besides, I modified the virsh_hostname function to satisfy more
> requirements.
>
> Signed-off-by: Gu Yanhua<guyanhua-f...@cn.fujitsu.com>
> ---
> client/virt/libvirt_vm.py | 19 ++++++++++++-------
> 1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/client/virt/libvirt_vm.py b/client/virt/libvirt_vm.py
> index eda5e8e..2734a6c 100644
> --- a/client/virt/libvirt_vm.py
> +++ b/client/virt/libvirt_vm.py
> @@ -86,7 +86,7 @@ def service_libvirtd_control(action):
> raise error.TestError("Unknown action: %s" % action)
>
>
> -def virsh_cmd(cmd, uri = ""):
> +def virsh_cmd(cmd, uri = "",raise_error = True, extra_param = ""):
Good, this is maintaining the existing behavior.
> """
> Append cmd to 'virsh' and execute, optionally return full results.
>
> @@ -100,10 +100,15 @@ def virsh_cmd(cmd, uri = ""):
> uri_arg = ""
> if uri:
> uri_arg = "-c " + uri
> - cmd = "%s %s %s" % (VIRSH_EXEC, uri_arg, cmd)
> - cmd_result = utils.run(cmd, verbose=DEBUG)
> - return cmd_result.stdout.strip()
> -
> + cmd = "%s %s %s %s" % (VIRSH_EXEC, uri_arg, cmd, extra_param)
I'm not sure why 'extra_param' would be needed, can't it just all get
packed into the cmd parameter before calling virsh_cmd()? I think it's
valuable to keep low-level functions as simple as possible. But if you
have a good reason for needing extra_param, then I'm okay with it.
> + cmd_result = utils.run(cmd, verbose=DEBUG, raise_error=raise_error)
I think we can just use 'ignore_status = not raise_error' or something
similar? I think this would have exact same behavior you desire w/o
adding any additional params to utils.run. Or am I missing someting?
> + logging.info("command:%s", cmd)
> + logging.debug("stdout: %s", cmd_result.stdout.strip())
> + logging.debug("exit_status: %s", cmd_result.exit_status)
These additional logging statements look fine to me. I don't think it
will be much of a burden, and when things go wrong they will help
debugging. We can always remove them later if the output gets to be too
much.
> + if raise_error:
> + return cmd_result.stdout.strip()
Existing virsh_cmd() callers are probably expecting an error.CmdError
exception to be thrown here. Maybe a better way is to wrap the
utils.run() call in a try, then test raise_error inside the except clause?
note: Remember not to use finally clause, not supported in older python :(
> + else:
> + return cmd_result.exit_status, cmd_result.stdout.strip()
What do you think about just returning cmd_result here? Then the caller
has the flexibility to decide what data is important and it keeps
virsh_cmd() simple.
>
> def virsh_uri(uri = ""):
> """
> @@ -112,11 +117,11 @@ def virsh_uri(uri = ""):
> return virsh_cmd("uri", uri)
>
>
> -def virsh_hostname(uri = ""):
> +def virsh_hostname(uri = "", raise_error = True, extra_param = ""):
> """
> Return the hypervisor hostname.
> """
> - return virsh_cmd("hostname", uri)
> + return virsh_cmd("hostname", uri, raise_error, extra_param)
Assuming you need 'extra_param' here for a test, this is fine. I think
for the virsh_cmd() call though, we could just use '"hostname%s" %
extra_param' or an equivalent, no? Again, I'm okay with having
'extra_param' in virsh_cmd() if there's a good reason.
>
>
> def virsh_version(uri = ""):
> --
> 1.7.1
Otherwise, it looks good to me, thanks!
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest mailing list
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
