On Tue, Oct 23, 2012 at 11:02:18AM -0700, Gurucharan Shetty wrote: > On Mon, Oct 22, 2012 at 1:52 PM, Ben Pfaff <[email protected]> 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 <[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}" > > > 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. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
