On Wed, Jun 08, 2016 at 08:02:29PM -0500, Ryan Moats wrote: > From: "RYAN D. MOATS" <rmo...@us.ibm.com> > > Ensure that ovn flow tables are persisted so that changes to > them chan be applied incrementally - this is a prereq for > making lflow_run and physical_run incremental. > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
Thanks for the patch. I have some comments. We don't really use // comments in OVS. Please change them to /**/ comments (beginning with a capital, ending with punctuation). This comment on ofctrl_add_flow() is now wrong: + * The caller should initialize its own hmaps to hold the flows. Comment on ofctrl_add_flow() says that there's an index on table_id+priority+uuid but I think it's actually on uuid only. Also, s/usess/uses/ in that comment. struct ovn_flow contains a struct uuid *. I don't understand how the function can guarantee that the pointer remains valid. It seems like the prudent thing to do would be to contain a struct uuid instead. (I've noticed the same thing in previous patches; it worried me there too but those seemed like less general cases.) ofctrl.c has a prototype for ovn_flow_table_clear() which duplicates the one in ofctrl.h. ovn_flow_lookup_by_uuid() is positively Lovecraftian. I don't understand what it's doing. In ovn_flow_destroy(), please don't bother to check for nonnull before calling free(), since free() does that itself. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
