[ovs-dev] [initscripts 5/5] Refactor initscripts into distro-independent and distro-specific pieces.

Andrew Evans aevans at nicira.com
Thu Jun 16 22:35:01 UTC 2011


Thanks, this looks like a big improvement. Some minor comments below:

On Wed, 2011-06-15 at 16:42 -0700, Ben Pfaff wrote:

>+The \fBstart\fR command starts up Open vSwitch.  It performs the
>+following tasks:

Can we remove the word "up" here?

>+Loads the Open vSwitch kernel module.  If this fails, and the Linux
>+bridge module is loaded but no bridges exists, it tries to unload the

Can we strike the "s" in "exists"?

> +If the Open vSwitch database file does not exist, it creates it.
> +If the database does exist, but it has an obsolete version, it
> +upgrades it to the last schema.

I think "last" could be misinterpreted to mean "previous" here. How
about "latest" or "current" instead?

> +\fBOpen_vSwitch\fR table.  Specifying this option multiple times adds
> +multiple key-pair pairs.

Should this be "key-value"?

> +.SH "The ``stop'' command"
> +.
> +.PP
> +The \fBstart\fR command starts up Open vSwitch.  If
> +\fBovs\-vswitchd\fR is running, kills it and waits for it to
> +terminate, then it does the same for \fBovsdb\-server\fR.

Did you mean "The stop command stops Open vSwitch" instead?

> +Because \fBforce\-kmod\-reload\fR internally stop and starts OVS, it
> +accepts all of the options accepted by the \fBstart\fR command.

Should this read "stops and starts" instead?

> +case $0 in
> +    */*) dir0=`echo "$0" | sed 's,/[^/]*$,,'` ;;
> +    *) dir0=./ ;;
> +esac

Is there some reason not to use 'dirname'?

> +    # Allow GRE traffic.
> +    iptables -I INPUT -p gre -j ACCEPT

What if iptables isn't installed, or if the kernel wasn't built with the
necessary netfilter options?

> +    #   - There is an internal interface for every bridge, whether it
> +    #     has an Interface record or not and whether the Interface
> +    #     record's 'type' is properly set or net.

s/or net/or not/ ?

> +etcdir=$sysconfdir/openvswitch                  # /etc/openvswitch

Incidentally, I applied this patch series and built and installed .debs
on Ubuntu 11/Natty, and my database ended up
in /usr/etc/openvswitch/conf.db rather than /etc/openvswitch/conf.db.
I'm not sure if it's related to these changes.

Also, git-am complained about trailing whitespace in this patch.





More information about the dev mailing list