Ben Pfaff <b...@ovn.org> wrote on 06/23/2016 04:33:19 PM:
> From: Ben Pfaff <b...@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 06/23/2016 04:33 PM > Subject: Re: [ovs-dev,v18,4/9] Persist ovn flow tables. > > 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). Sure, will wait to see how much of the patch set needs update first though... > > This comment on ofctrl_add_flow() is now wrong: > + * The caller should initialize its own hmaps to hold the flows. Yes, I missed that... > 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. I'll double check that and fix the typo. > 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.) If I remember correctly, I asked about the uuid pointer in the ovsdb record hanging around and you answered "yes it does." However, I'm happy to (a) replace this with a struct uuid and copy/free information as needed, and (b) craft a patch to do the same with the previous uses. > ofctrl.c has a prototype for ovn_flow_table_clear() which duplicates the > one in ofctrl.h. ...and it should/will go away... > ovn_flow_lookup_by_uuid() is positively Lovecraftian. I don't > understand what it's doing. Now *that* comment is worth framing... I've not had code called Lovecraftian in a *LONG* time... > In ovn_flow_destroy(), please don't bother to check for nonnull before > calling free(), since free() does that itself. ack. Will you be looking at the rest of the refactors or should I just resubmit those patches? Ryan _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
