>> +def run_virsh_version(test, params, env):
>> + """
>> + Test command: virsh version.
>> + """
>> +
>> + def virsh_version(option):
>> + command = "virsh version %s" % option
>> + status, output = commands.getstatusoutput(command)
>> + logging.info("Output: %s", output)
>> + logging.info("Status: %d", status)
>> + return status
>
> ^ Rather than the commands API, I'd like to see here a call to
> utils.run, since it already is supposed to throw an exception if the
> command returns exit code != 0, so that you don't have to explicitly
> check for status and throw TestFail exceptions.
>
Hi, I have read the utils.run function and have some problems about this.
This patch's aim is to test whether the "virsh version"command can be
used effectively. When we test using a wrong option , it will cause an
exception but it is right ,we should not throw the exception and stop
the process. So we return status to judge. if exit code != 0, it is ringt,
otherwise it is wrong.
>> +
>> + case_num = params.get("case_num")
>> +
>> + libvirtd = "on"
>> + if params.has_key("libvirtd") == True:
>> + libvirtd = params.get("libvirtd")
>> +
>> + if case_num == "1":
>> + option = params.get("options")
>> + status = virsh_version(option)
>> + if status != 0:
>> + raise error.TestFail("Can not get the virsh version")
>> +
>> + elif case_num == "2":
>> + option = params.get("options")
>> + status = virsh_version(option)
>> + if status == 0:
>> + raise error.TestFail("Use virsh version command successfully with an
>> invalid option")
>> +
>> + elif case_num == "3":
>> + if libvirtd == "on":
>> + raise error.TestError("Libvirtd service still started!")
>
> ^ Service libvirtd still active sounds like a better error message, or
> better, a context message. Please verify how to use error.context in
> client/common_lib/error.py and client/virt/tests/boot.py.
>
ok. I got it. Thanks!
>> + elif libvirtd == "off":
>> + libvirt_vm.libvirtd_stop()
>> + option = params.get("options")
>> + status = virsh_version(option)
>> + libvirt_vm.libvirtd_start()
>> + if status == 0:
>> + raise error.TestFail("Use virsh version command successfully with
>> libvirtd service stop")
>> +
>
> ^ It seems to me that all 3 blocks can be handled with a single logic block
followed, thanks.
>
> if check_libvirtd:
> if libvirtd_on:
> ...
> option = params.get("options")
> status = virs_version(option)
>
> if check_libvirtd:
> if libvirtd_on:
>
> So please look into combining the case logic into a single set of
> instructions.
>
> Thanks for the contribution, looking forward to see an updated version
> of the test,
>
> Lucas
>
>
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
