[ovs-dev] [PATCH] utilities: Implement ovs-vlan-test script

Ben Pfaff blp at nicira.com
Thu Dec 16 18:29:02 UTC 2010


Thanks for implementing this!  Some comments below.

On Wed, Dec 15, 2010 at 03:23:12PM -0800, Ethan Jackson wrote:
> This patch implements a script which may be used to check for
> connectivity issues caused by bugs in linux drivers relating to
> VLAN traffic.

s/linux/Linux/

> diff --git a/utilities/automake.mk b/utilities/automake.mk
> index 9a334e3..51c3f8e 100644
> --- a/utilities/automake.mk
> +++ b/utilities/automake.mk
> @@ -11,7 +11,10 @@ bin_SCRIPTS += utilities/ovs-pki utilities/ovs-vsctl
>  if HAVE_PYTHON
>  bin_SCRIPTS += utilities/ovs-pcap utilities/ovs-tcpundump
>  endif
> -noinst_SCRIPTS += utilities/ovs-pki-cgi utilities/ovs-parse-leaks
> +noinst_SCRIPTS += \
> +	utilities/ovs-pki-cgi \
> +	utilities/ovs-parse-leaks \
> +	utilities/ovs-vlan-test

Do we really not want to install this?  If we don't install it, then
potential users have to go out of their way to get a copy.  But if we do
install it, then they just have to install OVS.

(To install it, we'll need to add it to the lists of files in
debian/openvswitch-common.install and xenserver/openvswitch-xen.spec,
and probably similarly for the manpages.)

>  utilities_ovs_appctl_SOURCES = utilities/ovs-appctl.c
> diff --git a/utilities/ovs-vlan-test.1.in b/utilities/ovs-vlan-test.1.in
> new file mode 100644
> index 0000000..7153a59
> --- /dev/null
> +++ b/utilities/ovs-vlan-test.1.in
> @@ -0,0 +1,56 @@
> +.TH ovs\-vlan-test 1 "December 2010" "Open vSwitch" "Open vSwitch Manual"

Missing \ before -test

> +.
> +.SH NAME
> +\fBovs\-vlan\-test\fR \- check linux drivers for problems with vlan traffic

s/linux/Linux/, s/vlan/VLAN/.

> +.SH SYNOPSIS
> +\fBovs\-vlan\-test\fR [\fIoptions\fR] \fIcontrol_ip\fR \fIvlan_ip\fR

It looks to me that we should really provide two synopses:

  ovs-vlan-test [-s | --server] control_ip vlan_ip
  ovs-vlan-test -h | --help | -V | --version

A few of the manpages have multi-way synopses like this, so I guess you
can follow those examples.

There's a lib/common-syn.man file that you can .so to save typing.

> +.SH DESCRIPTION
> +The \fBovs\-vlan\-test\fR program may be used to check linux kernel drivers for

s/linux/Linux/

> +problems sending VLAN traffic which may occur when running Open vSwitch.
> +.
> +.SS "Client Mode"
> +An \fBovs\-vlan\-test\fR client may be run on a host to check for VLAN
> +connectivity problems.  The client must be able to establish HTTP connections
> +with an \fBovs\-vlan\-test\fR server located at the specified \fIcontrol_ip\fR
> +address.  UDP traffic sourced at \fIvlan_ip\fR should be tagged and directed out
> +the interface whose connectivity is being tested.
> +.
> +.SS "Server Mode"
> +To conduct tests, an \fBovs\-vlan\-test\fR server must be running on a host
> +known not to have VLAN connectivity problems.  The server must have a
> +\fIcontrol_ip\fR on a non-VLAN network which clients can establish connectivity
> +with.  It must also have a \fIvlan_ip\fR address on a VLAN network which
> +clients will use to test their VLAN connectivity.  Multiple clients may test
> +against a single \fBovs\-vlan\-test\fR server concurrently.

This description of the client and server modes is good as far as it
goes, but I didn't fully understand how to use the program.  What is a
"VLAN network" or "VLAN IP" and how do I set one up?  (I can guess, but
the manpage should give more details.)  Do I have to use OVS to set one
up?  i.e. if I use vconfig instead to set one up, does that tell me
whether OVS VLANs will work?

I think that the key here is that the packets need to go out on the wire
with an 802.1Q header.  It might be nice to mention that in passing
since the term VLAN is pretty overloaded.

I don't see anything about the format of the output or how to interpret
it.  Even if the program prints a really clear "TEST PASSED" or "TEST
FAILED" or whatever, the manpage should at least mention that.

> +.SH OPTIONS
> +.
> +.IP "\fB\-s\fR"
> +.IQ "\fB\-\-server\fR"
> +Run in server mode.

The .IQ macro needs to be defined at the top of the file; as-is, its
argument is being ignored entirely and that version doesn't appear.

You can .so the lib/common.man file to get text for --help and --version.

> +.IP "\fB\-h\fR"
> +.IQ "\fB\-\-help\fR"
> +Display a help message.
> +.
> +.IP "\fB\-V\fR"
> +.IQ "\fB\-\-version\fR"
> +Display the Open vSwitch version of this program.
> +.
> +.SH EXAMPLES
> +.TP
> +\fBovs\-vlan\-test\fR -s 0.0.0.0:80 1.2.3.4

The whole line should be bold, not just the program name.

> +Runs an \fBovs\-vlan\-test\fR server listening for client control traffic on
> +port 80 of any address and VLAN traffic on the default port of 1.2.3.4.
> +.
> +.TP
> +\fBovs\-vlan\-test\fR 5.6.7.8 1.2.3.4:99

Ditto here.

It would be nice to include a more extensive example that includes how
to configure OVS (if that's a requirement) or vconfig or whatever.

> +Runs an \fBovs\-vlan\-test\fR client with a control server located at the
> +default port of 5.6.7.8 and a local VLAN ip of 1.2.3.4 port 99.
> +.
> +.SH SEE ALSO
> +.
> +.BR ovs\-vswitchd (8),
> +.BR ovs\-ofctl (8),

The list shouldn't end with a comma.

> diff --git a/utilities/ovs-vlan-test.in b/utilities/ovs-vlan-test.in
> new file mode 100755
> index 0000000..61b6f3d
> --- /dev/null
> +++ b/utilities/ovs-vlan-test.in
> @@ -0,0 +1,441 @@
> +#! @PYTHON@

What version(s) of Python did you try this with?  Does it work as far
back as the Python 2.4 installed on XenServer 5.5.0?

> +#Caller is responsible for catching socket.error exceptions.
> +def send_packet(key, length, dest_ip, dest_port):
> +
> +    length -= 28 #L3 L4 headers.

That's 20 + 8 for IP + UDP?  Might just spell that out explicitly then,
so we don't have to guess.

Do we have any way to verify that the OS isn't adding anything else,
e.g. adding IP options etc.?  (Does it matter if it does?)

> +    packet   = [chr(0) for _ in range(length)]
> +    data_str = str(key)
> +
> +    for i in range(len(data_str)):
> +        packet[i] = data_str[i]
> +    packet = ''.join(packet)

Yowza, is that really the best way to construct a packet in Python?  I
had to read it a few times.

(The _ shows your functional programming background :-)

> +        success = [False] #Primitive datatypes can't be set from closures.

Hmm, interesting, I never realized that.  It is a side-effect of its
scoping rules.  An assignment within a block always introduces a local
variable, unless the name is declared global.  But there's no way to
declare a name as being that of an enclosing scope other than the global
scope.

I can't claim to have carefully read all of the Python code.  I skimmed
it.




More information about the dev mailing list