On Tue, Jun 16, 2015 at 01:07:36AM -0700, Alex Wang wrote:
> Really like this patch~!
Thanks!
> > + <dd>
> > + Adds a new port to <var>bridge</var> in the default sandbox (as
> > set
> > + with <code>as</code>) that plugs it into the <var>network</var>
> > + interconnection network. <var>network</var> must already have
> > been
> > + created by a previous invocation of <code>net_add</code>. The
> > default
> > + sandbox must not be <code>main</code>.
> > + </dd>
> >
>
>
> First sentence of the description of 'net_attach' is confusing. do you mean
> 's/that/and/'?
That works OK for me too, so I made the change.
> > +for option; do
> > + # This option-parsing mechanism borrowed from a Autoconf-generated
> > + # configure script under the following license:
> > +
> > + # Copyright (C) 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2001,
> > + # 2002, 2003, 2004, 2005, 2006, 2009, 2013 Free Software Foundation,
> > Inc.
> > + # This configure script is free software; the Free Software Foundation
> > + # gives unlimited permission to copy, distribute and modify it.
> > +
> > + # If the previous option needs an argument, assign it.
> > + if test -n "$prev"; then
> > + eval $prev=\$option
> > + prev=
> > + continue
> > + fi
>
> "prev" not used, i think the above code can be removed?
There's a lot that can go. I removed it all, as well as the copyright
notice since I took out everything that came from Autoconf.
> > +# Check that we've got proper builddir and srcdir.
> > +if test ! -e "$sim_builddir"/vswitchd/ovs-vswitchd; then
> > + echo "$sim_builddir/vswitchd/ovs-vswitchd does not exist (need to run
> > \"make\"?)" >&2
> > + exit 1
> > +fi
> > +if test ! -e "$sim_srcdir"/WHY-OVS.md; then
> > + echo "$sim_srcdir/WHY-OVS.md does not exist" >&2
> > + exit 1
> > +fi
>
> Why WHY-OVS? ;D
The goal is to look for a file that is in the OVS source tree and
unlikely to be in any other source tree, so WHY-OVS seems like a good
choice to me.
> > +# Put built tools early in $PATH.
> > +if test ! -e $sim_builddir/vswitchd/ovs-vswitchd; then
> > + echo >&2 'build not found, please change set $sim_builddir or change
> > directory'
> > + exit 1
> > +fi
>
> Duplicated check?
Thanks, dropped.
> > +PATH=$sim_builddir/ovsdb:$sim_builddir/vswitchd:$sim_builddir/utilities:$PATH
> >
> > +PATH=$sim_builddir/ovn:$sim_srcdir/ovn:$sim_builddir/ovn/controller:$sim_builddir/ovn/northd:$PATH
> > +export PATH
> > +
> > +rm -rf sandbox
> > +mkdir sandbox
> > +cd sandbox
> > +sim_base=`pwd`; export sim_base
> > +trap "cd '$sim_base' && kill \`cat */*.pid\`" 0 1 2 3 13 14 15
> > +
> > +sim_setvars() {
> > + sandbox=$1
> > + OVS_RUNDIR=$sim_base/$1; export OVS_RUNDIR
> > + OVS_LOGDIR=$sim_base/$1; export OVS_LOGDIR
> > + OVS_DBDIR=$sim_base/$1; export OVS_DBDIR
> > + OVS_SYSCONFDIR=$sim_base/$1; export OVS_SYSCONFDIR
> > + PS1="|$1: $sim_PS1"
> > +}
>
> Do you want to add a space between for PS1? (i.e., PS1="| S1: $sim_PS1")
I think it looks OK without the space.
> > +export -f sim_setvars
> > +
> > +as() {
> > + case $# in
> > + 0)
> > + echo >&2 "$FUNCNAME: missing arguments (use --help for help)"
> > + return 1
> > + ;;
> > + 1)
> > + if test "$1" != --help; then
> > + sim_setvars $1
> > + else
> > + cat <<EOF
> > +$FUNCNAME: set the default sandbox for Open vSwitch commands
> > +usage: $FUNCNAME SANDBOX [COMMAND ARG...]
> > +where SANDBOX is the name of the desired sandbox.
> > +
> > +With COMMAND arguments, this command sets the default target for that
> > +single command, which it runs directly. Otherwise, it sets the default
> > +target for all following commands.
> > +EOF
>
> Do you want to prefix all usage prints with '| '? To keep consistency?
I would rather not. It would be a hassle if we ever wanted to change
it, and other commands that the user runs don't have the prefix anyway.
> > + set X $1; shift
> > + if test $# != 1; then
> > + echo >&2 "$FUNCNAME: sandbox name must be a single word"
> > + return 1
> > + fi
> > +
> >
>
>
> This is a cool check~
Thanks.
I think that this is a positive review, so I'm going to push this to
'ovn' in a minute.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
