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

[Gurucharan Shetty]

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.


>
> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121023/20293259/attachment-0003.html>