[ovs-dev] [PATCH 08/23] ovn-controller: Factor patch port management into new "patch" module.

[Ben Pfaff]

On Fri, Oct 16, 2015 at 08:15:19AM +0000, Justin Pettit wrote:
> 
> > On Oct 15, 2015, at 9:35 PM, Ben Pfaff <blp at nicira.com> wrote:
> > 
> > On Thu, Oct 15, 2015 at 11:38:34PM +0000, Justin Pettit wrote:
> >> After reviewing patch 10, I have a few more thoughts on this patch.  This doesn't change my original ACK, though.
> > 
> > This patch is supposed to be mostly moving code around.  I'm happy to
> > make the improvements you mention but I'd like to separate them from
> > just moving code from ovn-controller.c into a new file. 
> > 
> >>> +static void
> >>> +create_patch_port(struct controller_ctx *ctx,
> >>> +                  const char *network,
> >>> +                  const struct ovsrec_bridge *b1,
> >>> +                  const struct ovsrec_bridge *b2)
> >>> +{
> >> 
> >> This could use a comment and maybe a renaming of some arguments.  In
> >> particular, it isn't obvious the importance between "b1" and "b2".
> >> Also, it might be worth noting that it only creates one side of the
> >> patch port.
> > 
> > The later commit "patch: Allow client to determine port names." renames
> > b1 and b2 to src and dst.  It doesn't add a comment; I'll take care of
> > that though.
> 
> Yeah, I noticed that in the later patch.  Thanks.
> 
> >>> +static void
> >>> +parse_bridge_mappings(struct controller_ctx *ctx,
> >>> +                      const struct ovsrec_bridge *br_int,
> >>> +                      const char *mappings_cfg)
> >> 
> >> This function does a lot more than just parse the bridge mappings.  It
> >> could probably use a better name, but, at the very least, I think a
> >> comment would be helpful--especially for the non-obvious things like
> >> what will happen to "mappings_cfg".
> > 
> > OK, I'll add a comment.
> > 
> > But what's nonobvious about mappings_cfg?
> 
> Oh, nothing.  I was thinking about parse_bridge_mappings()'s later
> incarnation that takes "existing_ports" as an argument instead and
> then modifies it in place.  This one is clear, but that later version
> is not.

OK, I made sure that the later version now has a comment about what
happens there.