Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virttest.utils_net: Move method ip_link_ctl from macvtap class #1593

Closed

Conversation

Xiangmin
Copy link
Contributor

Change ip_link_ctl as global method in utils_net.

Signed-off-by: Xiangmin Han xhan@redhat.com

Change ip_link_ctl as global method in utils_net.

Signed-off-by: Xiangmin Han <xhan@redhat.com>
@Xiangmin
Copy link
Contributor Author

@FengYang
I have checked tp-libvirt, tp-qemu and virt-test, the ip_link_ctl function is only called by the class macvtap methods. At the same, I have changed the callers which would use ip_link_ctl to use the
global one.

@zhouqt
Copy link
Member

zhouqt commented Apr 16, 2014

Hey @Xiangmin,

I'd ask why you need to put these functions global? IMO, you break the cohesion [1] of a class, I even prefer copying these function into your own script, to separating them from a class.

So please, is it the only way to make them global?

Thanks,
Tom

[1] http://en.wikipedia.org/wiki/Cohesion_(computer_science)

@ypu
Copy link
Member

ypu commented Apr 16, 2014

@zhouqt Agree with you about the break cohesion part. Just look through the code and find that ip_link is quite basic function for interface ops. So I guess we can put it from Macvtap class to the basic class Interface and use it through that class.

@Xiangmin
Copy link
Contributor Author

Thanks @ypu and @zhouqt
I would change and send again.

@lmr
Copy link
Member

lmr commented Apr 17, 2014

@Xiangmin, please make the changes and rebase. this will allow your PR to be tested by Travis.

@Xiangmin
Copy link
Contributor Author

Would add delete function in the original Interface class. So close this PR.

@Xiangmin Xiangmin closed this Apr 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants