On Tue, Oct 18, 2011 at 09:23:46PM -0700, Ansis Atteka wrote:
> ovs-vlan-test runs through a number of tests to identify VLAN issues. This
> is useful when trying to debug why a particular driver has issues, but it made
> the testing environment a bit harder to set up. This commit adds an iperf
> test to check basic functionality. It also useful in detecting performance
> issues.
>
> Issue #6976
Thank you for doing this.
"git am" says:
Applying: ovs-vlan-test: Add iperf to test basic connectivity.
/home/blp/db/.git/rebase-apply/patch:32: trailing whitespace.
The \fBovs\-vlan\-test\fR program automates \fBiperf\fR utility and allows
to
/home/blp/db/.git/rebase-apply/patch:33: trailing whitespace.
detect connectivity and performance problems. These problems can occur when
/home/blp/db/.git/rebase-apply/patch:34: trailing whitespace.
Open vSwitch is used to send 802.1Q traffic through physical interfaces
running
/home/blp/db/.git/rebase-apply/patch:35: trailing whitespace.
certain drivers of certain Linux kernel versions. To run a test, configure
Open
/home/blp/db/.git/rebase-apply/patch:36: trailing whitespace.
vSwitch to tag traffic originating from interface where
\fBovs\-vlan\-test\fR
warning: squelched 25 whitespace errors
warning: 30 lines add whitespace errors.
pychecker says:
ovs-vlan-test.py:46: Comparisons with False are not necessary and may
not work as expected
ovs-vlan-test.py:49: Comparisons with True are not necessary and may
not work as expected
ovs-vlan-test.py:96: Local variable (t) not used
pep8 says:
ovs-vlan-test.in:33:1: W293 blank line contains whitespace
ovs-vlan-test.in:35:80: E501 line too long (80 characters)
ovs-vlan-test.in:35:45: E261 at least two spaces before inline comment
ovs-vlan-test.in:42:42: W291 trailing whitespace
ovs-vlan-test.in:59:71: E225 missing whitespace around operator
ovs-vlan-test.in:126:37: E231 missing whitespace after ','
ovs-vlan-test.in:168:62: E203 whitespace before ','
ovs-vlan-test.in:173:41: E702 multiple statements on one line (semicolon)
I think that this warrants an item in NEWS, saying that ovs-vlan-test
has been improved.
In ovs-vlan-test.8.in, please don't write variable names like
SERVERPORT in all-caps. That makes them harder to read; the italic
font is good enough. You can also put in a dash to break up words,
e.g. server-port. And spaces help readability too.
But I'm not sure where the SERVERPORT and SERVERIPPORT names come from
anyway. These are just a port number and an ip:port pair, right? I
think so. And TARGETBANDWIDTH is a bit of a mouthful, and it looks
like one can suffix it with K or M anyway.
So, I would write this:
\fBovs\-vlan\-test\fR [\fB\-s\fR \fISERVERPORT\fR|\fB\-c\fR
\fISERVERIPPORT\fR]
[\fB\-b\fR \fITARGETBANDWIDTH\fR]
as something more like:
\fBovs\-vlan\-test\fR [\fB\-s\fR \fIport\fR | \fB\-c\fR \fIip\fB:\fIport\fR]
[\fB\-b\fR \fIrate\fR[\fBK\fR | \fBM\fR]
The capitalized names should be italicized the other places they
appear, too. The general rule is that literal text that the user must
type is in bold, variables that the user must replace with a correct
value are in italics, and operators like [ ] and | are in a plain
font. man(1) has some details, at least on my system.
I think that more idiomatic than:
if ("connect failed" in self.err_data) or \
("Connection refused" in self.err_data) or \
("did not receive ack" in self.err_data):
would be:
if ("connect failed" in self.err_data or
"Connection refused" in self.err_data or
"did not receive ack" in self.err_data):
In general I think we usually prefer to use parentheses instead of \
to continue lines.
How much output does iperf print? Python isn't very good at
concatenating strings one at a time, so if it prints a significant
amount of output it's better to put the strings into a list and
concatenate them all at once in the end with ''.join(list).
Your new ClientTestScheduler class should have "object" as its
superclass.
Ethan or Reid might have more comments.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
