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<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.
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!
>
>>
>> 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
Autotest@test.kernel.org
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
