On 05/24/2012 06:03 AM, Alex Jia wrote:
> On 05/24/2012 03:39 PM, tangchen wrote:
>> This patch adds 3 API for libvirt_vm.
>> 1) vm.define()
>> 2) vm.attach_interface()
>> 3) vm.detach_interface()
>>
>> Signed-off-by: Tang Chen<[email protected]>
>> ---
>> client/virt/libvirt_vm.py | 80
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 80 insertions(+), 0 deletions(-)
>>
>> diff --git a/client/virt/libvirt_vm.py b/client/virt/libvirt_vm.py
>> index eda5e8e..5d35888 100644
>> --- a/client/virt/libvirt_vm.py
>> +++ b/client/virt/libvirt_vm.py
>> @@ -432,6 +432,62 @@ def virsh_migrate(options, name, dest_uri, extra,
>> uri = ""):
>> return True
>>
>>
>> +def _check_interface_type(type):
>> + if not type:
>> + logging.error("Interface type missing.")
>> + return False
>> +
>> + if type not in ('bridge', 'network', 'user', 'ethernet', 'direct',
>> + 'hostdev', 'mcast', 'server', 'client'):
>> + logging.error("Unknown interface type %s", type)
>> + return False
>> +
>> + return True
>> +
>> +
>> +def _build_run_at_dt_interface_cmd(cmd, type, extra="", uri=""):
>> + if not _check_interface_type(type):
>> + return False
> As usual, I also do this like you, however, from test POV, it's a wrong
> behavior,
> we shouldn't block invalid interface type in our codes, it's also a test
> scenario.
>
> For example, if we directly give a invalid type to virsh then I assume
> virsh probably
> will crash, but if we call the function before running virsh cmd, it can
> only raise
> your error then we will miss a important test scenario.
Yes, I agree also. It's good programming practice to validate input,
and this is what you have done here. However Alex is right, we're also
interested in testing libvirt/virsh validation, which includes invalid
input. So, it would be better to let libvirt/virsh handle the
validation, and if there's a problem, just let the exception propigate
and fail the test (or catch the failure if testing for it).
The "rule" I've been using that helps me decide is: It's okay and good
to validate, when the test code is acting as a marshal between two
interfaces. For example, A specialized Cartesian param. that acts as
input for multiple objects (i.e. vm_type is a good example). Otherwise,
if it's basically a pass-through option to the "thing" which is being
tested, it's better (and simpler) to just pass the option through
no-questions-asked.
Make sense?
>> +
>> + if cmd.startswith("attach-interface"):
>> + verb = "attach"
>> + elif cmd.startswith("detach-interface"):
>> + verb = "detach"
>> + else:
>> + logging.error("Unknown command: %s" % cmd)
>> +
>> + cmd += " --type %s" % type
>> +
>> + if extra:
>> + cmd += " %s" % extra
>> +
>> + try:
>> + virsh_cmd(cmd, uri)
>> + return True
>> + except error.CmdError, detail:
>> + logging.error("Interface %s on domain %s failed:\n%s",
>> + verb, name, detail)
>> + return False
>> +
>> +
>> +def virsh_attach_interface(name, type, extra="", uri=""):
>> + """
>> + Attach a nic to VM.
>> + """
>> + cmd = "attach-interface --domain %s " % name
>> +
>> + return _build_run_at_dt_interface_cmd(cmd, type, extra, uri)
>> +
>> +
>> +def virsh_detach_interface(name, type, extra="", uri=""):
>> + """
>> + Detach a nic to VM.
>> + """
>> + cmd = "detach-interface --domain %s " % name
>> +
>> + return _build_run_at_dt_interface_cmd(cmd, type, extra, uri)
>> +
>> +
>> def virsh_attach_device(name, xml_file, extra = "", uri = ""):
>> """
>> Attach a device to VM.
>> @@ -556,6 +612,16 @@ class VM(virt_vm.BaseVM):
>> return False
>>
>>
>> + def define(self, path):
>> + """
>> + Define the VM.
>> + """
>> + if not os.path.exists(path):
>> + logging.error("File %s not found." % path)
>> + return False
>> + return virsh_define(path, self.connect_uri)
>> +
>> +
>> def undefine(self):
>> """
>> Undefine the VM.
>> @@ -1234,6 +1300,20 @@ class VM(virt_vm.BaseVM):
>> return result
>>
>>
>> + def attach_interface(self, type, extra = ""):
>> + """
>> + Attach a nic to VM.
>> + """
>> + return virsh_attach_interface(self.name, type, extra, self.connect_uri)
> attach_interface()->virsh_attach_interface()->_build_run_at_dt_interface_cmd(),
> maybe,
> we may reduce some calling level, otherwise, need to wrapper 3 function
> in order to
> use a virsh command, of course, I know you want to abstract some method
> then call it.
I also agree. For me, since the virsh_*() functions you've added are
very simple, 1-3 lines, I think they can just be rolled up the stack
into the vm.*_interface() methods. If we need more abstraction (or if
you have another test that uses it) then what you have here is fine.
Otherwise, this is abstraction which can easily be added back in later
if/when it's actually needed.
>> +
>> +
>> + def detach_interface(self, type, extra = ""):
>> + """
>> + Detach a nic from VM.
>> + """
>> + return virsh_detach_interface(self.name, type, extra, self.connect_uri)
>> +
>> +
>> def attach_device(self, xml_file, extra = ""):
>> """
>> Attach a device to VM.
>
--
Chris Evich, RHCA, RHCE, RHCDS, RHCSS
Quality Assurance Engineer
e-mail: cevich + `@' + redhat.com o: 1-888-RED-HAT1 x44214
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
