[ovs-dev] [PATCH 1/2 v2] ovs-ctl.in: Ability to save flows and kernel datapath config.

Ben Pfaff blp at nicira.com
Tue Oct 23 18:04:40 UTC 2012


On Tue, Oct 23, 2012 at 11:02:18AM -0700, Gurucharan Shetty wrote:
> On Mon, Oct 22, 2012 at 1:52 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > On Fri, Oct 19, 2012 at 06:32:14PM -0700, Gurucharan Shetty wrote:
> > > Add a new command - "restart" to ovs-ctl. Calling this command
> > > will save and restore the Openflow flows on each bridge while
> > > stopping and starting the userspace daemons respectively.
> > >
> > > Also, during a force-reload-kmod, save the flows and kernel datapath
> > > configuration. Use the saved datapath configuration while readding
> > > the kernel module and the flows while starting the userspace daemons.
> > >
> > > Feature #13555.
> > > Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
> >
> > There are two instances of code like:
> > +    [ -n "${script_datapaths}" ] && \
> > +        action "Restoring datapath configuration" "${script_datapaths}"
> > that might better be factored out into a function.
> >
> 
> > Similarly for:
> >     [ -n "${script_flows}" ] && \
> >         action "Restoring saved flows" "${script_flows}"
> >
> Okay.
> 
> >
> > I don't think that, as written, save_flows will ever return failure as
> > tested by this:
> >     if action "Saving flows" save_flows; then
> >         chmod +x "${script_flows}"
> >     else
> >         script_flows=
> >         log_warning_msg "Failed to save flows."
> >     fi
> > because the last command in save_flows is a variable assignment (that
> > always succeeds).  Also this code is repeated a couple of times, so
> > one might as well integrate it into save_flows itself.  I think we'd
> > end up with something like this:
> >
> The error checking was mostly a sanity check for the non-existence of
> ovs-save or a function inside ovs-save.
> 
> 
> >
> > save_flows () {
> >     if set X `ovs_vsctl list-br`; then
> >         shift
> >         if "$datadir/scripts/ovs-save" save-flows "$@" > "$script_flows";
> > then
> >             chmod +x "$script_flows"
> >             return 0
> >         fi
> >     fi
> >     script_flows=
> >     log_warning_msg "Failed to save flows."
> >     return 1
> > }
> >
> I will incorporate this.
> 
> 
> >
> > This box is a little misshapen:
> > > +# --------------- ##
> > > +## restart ##
> > > +## --------------- ##
> >
> I will correct this.
> 
> >
> > In ovs-save, I don't think it's a good idea to slurp all the ovs-ofctl
> > output into a shell variable, because there can be megabytes of it in
> > extreme cases:
> > > +        flows="`ovs-ofctl dump-flows "${bridge}" | sed -e
> > '/NXST_FLOW/d' \
> > > +            -e 's/idle_age=[^,]*,//'`"
> > > +        echo "ovs-ofctl add-flows ${bridge} - << EOF
> > > +${flows}
> > > +EOF"
> > Instead I'd echo the line before the flows, call ovs-ofctl directly
> > (filtering through sed), and then echo the line after the flows.
> >
> I will change this.
> 
> 
> >
> > A lot of ovs-save looks a little funny in my text editor.  I think
> > that you must be using 4-space hard tabs?  It would be better to
> > indent with spaces, I think.  (CodingStyle requires that for C code;
> > we don't have an official coding style for shell.)
> >
> My .vimrc did not do what I thought it should do. Apparently it doesn't
> replace tabs by spaces automatically if I do a mass code indentation
> adjustment. Sorry about it.
> 
> 
> >
> > In save_datapaths, the use of ! is not portable shell (even though
> > it's in POSIX), so please write it as "if <condition>; then :; else
> > <code>; fi" instead:
> >                 if ! port_no=`expr "${line}" : '.*port \([0-9]\+\):'`; then
> > Or you could use port_no=`...` || continue.
> >
> I will correct this.
> 
> 
> >
> > I think you could simplify the inner "read" loop in save_datapaths a
> > bit by making use of the "read" command's ability to break a line up
> > into words rather than postprocessing it into words with "awk".
> >
> > It might be nice to give an example of the kinds of line we're trying
> > to process in a comment, to make the parsing code easier to read.
> >
> My first attempt was to do a word by word processing instead of line
> by line. It got my code a little more complicated and messy.
> I will provide an incremental with an example of ovs-dpctl show. If you
> feel that word by word is still a better option, I will give it another
> shot.

All this sounds good, thank you.



More information about the dev mailing list