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 <[email protected]> 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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://fedorahosted.org/mailman/listinfo/vdsm-patches
