this automates 9 commands of virsh iface #681
Conversation
…into virsh_interface1
|
Wa oh...You have done so many test cases about virsh iface-*...I was thinking about it. Ok, I will share my ideas with you if I think it's helpful for your test.Thanks! |
|
I just saw some codes..maybe you have to do some work on their format :) |
| if os.path.exists("%s%s"%(net_scr(),opt)): | ||
| return 'yes' | ||
| else: | ||
| return 'no' |
There was a problem hiding this comment.
I think one line is enough, Return a bool value is better than string..
return os.path.exists("%s%s"%(net_scr(),opt))Then you can use it like:
if is_scr_avail(opt):
pass
else:
passThere was a problem hiding this comment.
I addressed this in my latest patches. Please have a look
| return False | ||
| else: | ||
| return True | ||
| #print avail_vir_dbl_ifaces('br0','eth0') |
There was a problem hiding this comment.
I found there are some #print , maybe you used them for debugging and forgot to remove them
|
@sanjeevpatro You need to run |
| @param: to_file: capture the output to a optional file | ||
| @return: CmdResult object | ||
| """ | ||
| cmd = ('iface-dumpxml %s %s %s' %(ifc_name,to_file,extra)) |
There was a problem hiding this comment.
It seems there is no need to add to_file into cmd.
|
Well, the virsh part looks much better, and I only found a little issue which I believe only takes you 1 min to fix. good job. And I will take a look at your code for test module. Thanx. |
| elif options_ref == '--inactive': | ||
| return check_virsh_list_inactive() | ||
| else: | ||
| return check_virsh_list_active() |
There was a problem hiding this comment.
Oh man ,maybe you misunderstant autotest :(
your codes will always pass, because they just return True or False.
Actually when test failed, you'd better raise an exception, you can see other test files
|
I tried to incorporate all the feedback, whatever I got it from liyangfnst and yangdongsheng |
|
If required I ll post some snippets of logging debug of any one command |
| #!/usr/bin/python | ||
| import os,logging, fileinput, re | ||
| from autotest.client.shared import utils,error | ||
| from virttest import virsh |
There was a problem hiding this comment.
@sanjeevpatro
I found you add a new module named iface to manager the all interafces.
But I am afraid it is not the correct location.
There is a dir virttest/libvirt_xml/. Maybe you can take a look at this module and do this work in it.
Thanx for your update.
There was a problem hiding this comment.
Does that mean shall i move the iface module from virttest to virttest/libvirt_xml/ and update path in all other runner scripts
There was a problem hiding this comment.
I think we do not need a new module for iface. Lots of the functions are getting or setting info in xml. I believe they should be
virttest/libvirt_xml and the others need be added into another module such as utils_net, virsh or utils_misc. (I am not clear about
every function in your patch.)
In addition, I sugguest you to read some modules in virttest/libvirt_xml. There are some specific coding style you need to follow.
Hmmm, it seems there are lots of work we need to do before apply this large patchset. My suggestion is splite this patch into
some small pull-requests. We can discuss the all problem in first patchset and then the others will be easy.
e.g:
we can focus on virsh_iface_define_undefine test. Add the apis this test will use in the correct module. Then I believe the other
tests is very similar work with it.
|
@sanjeevpatro @yangdongsheng Hmmm. I have a suggestion. Since there is no relationship between these commands, I suggest you close this pullreq and send these patches one by one, for example, you can send |
|
@yangdongsheng Wooo... almost same time to give @sanjeevpatro a same idea, hehe |
|
The main reason is I wanted to segregate all the function required by virsh_iface module, other than virsh commands, in single file. At the same time I could not get much idea where to add on those functions in existing modules, but i tried to see if it is available or not but did not get any satisfactory result. I felt it is the easy path. I am Ok with start up new pull request with, virsh_iface_list first, where I tried to touch up on virsh iface-list commands and tried to store the output in a list base dictionary. All my scripts would traverse through this. And let us go by the iface module, where I will try to explain more about each and every module and see where we can accommodate. Instead of cancelling pull request is it possible to push this, later as work goes by we will replace those scripts with newer ones. As of I know bare minimal things are working fine. Understand change of the coding standard essential, any how I will work upon that and try to give you the best results The last one is just a suggestion, ignore if it not relevant |
|
We can close this pull request, I will open another new pull request with "virsh iface-list" with only 4 files virsh iface-list.py, virsh iface-list.cfg, virsh.py and iface.py(optional) from next week. Thanks for all the feedback I will try to improve more on this, specifically the coding standard. Need your cooperation. Let me any write up required to understand the logical flow? |
|
Thanks for your response, fair enough to send a new pullreq, and I'm also interesting in these |
|
@sanjeevpatro Closing it and watching a new pull request. Thanx |
|
Hi, @sanjeevpatro , I saw you have did some work on some virsh iface-* command, and I want to know did you have begin some test on virsh iface-dumpxml and iface-edit, hope your response :) |
|
Hey @sanjeevpatro , what's interface commands going on? |
|
On 01/22/2014 02:52 PM, Fergus Yu wrote:
Thanks |
It automates 9 command by their respective file
File - virsh_iface_list.py
virsh iface-list
File - virsh_iface_start_destroy.py
virsh iface-destroy
virsh iface-start
File - virsh_iface_define_undefine.py
virsh iface-define
virsh iface-undefine
File - virsh_iface_mac_name.py
virsh iface-mac
virsh iface-name
File- virsh_iface_bridge_unbridge.py
virsh iface-bridge
virsh iface-unbridge
Library file-iface.py
virsh command file - virsh.py