Change in vdsm[master]: Basic tests for the tc module
danken at redhat.com
danken at redhat.com
Fri Jul 13 19:42:38 UTC 2012
Dan Kenigsberg has posted comments on this change.
Change subject: Basic tests for the tc module
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(9 inline comments)
Thank you very much, Roman, for this contribution. I have several minor comments, and one big request: if you can verify that a ping into bridge_1 is actually mirrored into bridge_2.
....................................................
File AUTHORS
Line 33: Roman Fenkhuber <roman at fenkhuber.at>
please keep sorted.
....................................................
File tests/Makefile.am
Line 28: tcTests.py \
please keep sorted
Line 47: vdsClientTests.py
hmm, it's not quite sorted... but still.
....................................................
File tests/tcTests.py
Line 48: class TestProcessRequest(TestCaseBase):
_process_request is a badly-named function that should be kept as a private implementation detail. I do not see the grand benefit of testing it here.
Line 52: _process_request(['/usr/bin/echo', '-n', 'hello']))
If you decide to keep this test, please use EXT_ECHO = "/bin/echo"
so it works on my Fedora ;-)
Line 99: self.assertTrue('PROMISC' in _process_request([EXT_IFCONFIG,
could you use
ethtool.get_flags(self.bridge) & ethtool.IFF_PROMISC
for that? We are trying to get rid of EXT_IFCONFIG.
Line 115: @ValidateRunningAsRoot
I think it's enough to perform this validation on setUp. if it fails there, it won't be run here.
Line 122: tc.qdisc_add_ingress(self.bridge_1)
I'd prefer to test the higher-level function, tc.setPortMirroring instead of copying two of its lines to here.
Line 126: self.assertTrue("mirred" in filter)
Could you at least add a
TODO: verify that traffic on bridge_1 is mirrored to bridge_2
? if you can actually implement the test (ping + tcpdump?) it would be even better.
--
To view, visit http://gerrit.ovirt.org/6225
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee9437d1b5a96b3896df157f13888485ae7292d2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Roman Fenkhuber <roman at fenkhuber.at>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
More information about the vdsm-patches
mailing list