On 06/04/2012 11:08 AM, guyanhua wrote: > at 6/1/2012 10:50 PM, Chris Evich wrote: >> 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<[email protected]> >>> --- >>> 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. > I also hope so, but some virsh_cmd() callers' > return-result(cmd_result.stdout.strip()) > has already been directly used in other functions, thus changing-work > will be huge! If the old virsh_cmd() can't meet our requirement, we should improve it ASAP, at present, libvirt-autotest hasn't many test cases, otherwise, as you said, I will become a huge work in the future if we don't do it now. thanks. >>> 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! >> > _______________________________________________ > Autotest mailing list > [email protected] > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
_______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
