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

[Ben Pfaff]

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}"

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:

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
}
    
This box is a little misshapen:
> +# --------------- ##
> +## restart ##
> +## --------------- ##

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.

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.)

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 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.

Thanks,

Ben.