On Mon, Apr 2, 2012 at 1:28 PM, Ben Pfaff <b...@nicira.com> wrote: > On Fri, Mar 30, 2012 at 01:10:26PM -0700, Ansis Atteka wrote: > > -Implemented support for ovs-test client, so that it could automatically > > spawn an ovs-test server process from itself. This reduces the number of > > commands the user have to type to get tests running. > > -Automated creation of OVS bridges and ports (for VLAN and GRE tests), > so that > > user would not need to invoke ovs-vsctl manually to switch from direct, > 802.1Q > > and GRE tests. > > -Fixed some pylint reported warnings. > > -Fixed ethtool invocation so that we always try to query the physical > interface > > to get the driver name and version. > > -and some others enhancments. > > > > The new usage: > > Node1:ovs-test -s 15531 > > Node2:ovs-test -c 127.0.0.1,1.1.1.1 192.168.122.151,1.1.1.2 -d -l 125 -t > gre > > > > Signed-off-by: Ansis Atteka <aatt...@nicira.com> > > If we're lucky, Reid will review this also. > > NEWS should mention the enhancements. > > Is the special case for 127.0.0.1 useful? (Why would I want to run > this test against localhost?) > "127.0.0.1 case" allows you to type 2 commands:
Node1:ovs-test -s 15531 > Node2:ovs-test -c 127.0.0.1,1.1.1.1 192.<Node1>,1.1.1.2 -d -l 125 -t gre > instead of 3 commands: Node1:ovs-test -s 15531 > Node2:ovs-test -s 15531 > Node2:ovs-test -c <Node2>,1.1.1.1 <Node1>,1.1.1.2 -d -l 125 -t gre > I think that we usually use the style of two blank lines between > top-level definitions, but ip_optional_mask() only has one. > > vlan_tag(): Google Python style also says "if x or y", not "if (x) or (y)". > > vlan_tag(): "Should" => "should" > > xmlrpc_get_my_address_from(): "th" => "the" > > xmlrpc_create_test_bridge(): I don't understand in what sense this > function is atomic. Maybe "atomic" meaning is not appropriate here. With "atomic" I meant that we must do everything in one shot. Otherwise, If we would do multiple RPC calls just to create the Physical bridge (e.g. 1. creating bridge; 2. attaching physical interface to it; 3. moving IP config), then after step #2 the ovs-test client wouldn't be able to connect to ovs-test server anymore. > But it also seems like it should un-assign the IP > address from the original interface. > I though about the same thing, but this "lazy way" of not unassigning the IP address from the physical interface seemed to always work fine. But I do have to agree with you that this does not look nice, so I will try to fix it. > "bridge" is misspelled as "brige" in several places (even function > names). > > xmlrpc_create_bridge() and other functions check for != -1 but I'd > expect that the correct check is == 0. > > I would make xmlrpc_is_ovs_bridge() return True or False, not 0 or 1, > because the usage "if server.is_ovs_bridge(interface_name)== 0:" looks > funny. > > python/ovstest/vswitch.py has a ton of code duplication, some shared > with python/ovstest/util.py. I'd factor it out and add error > reporting. > > In the documentation, please write ethtool as \fBethtool\fR(8). > > In the documentation, I think that it would be nice to explain why one > would pick a particular innerip or innerport. My guess, without > thinking too hard, is that it is important that these inner ips do not > interfere with any IP addresses in use on either the server or the > client, because even though the data is tunnelled the inner IP traffic > still goes through the network stack on the client and the server. I > guess that the inner ports are not as important. > > In the examples, I think that it would be nice to explain the > suggested client command line a bit more. I think that it will run > three tests, one for direct mode, one for vlan mode, and one for gre > tunnel mode, but it would be nice to say that. > > On the same note, the manpage doesn't really say (that I noticed) what > happens when you give multiple mode options. It's common for > utilities to only support a single mode in one invocation, but I don't > think that this is the case here, so it might be good to say that. > You could put the options that add another mode into a subsection > (.SS) that mentions that each one introduces a new mode. > > Thanks, > > Ben. > Ok, to everything else and I will resubmit the patch. Sorry that this time I did not create multiple logical patches to make the review easier.
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
