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 <[email protected]>
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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
