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

Ben Pfaff blp at nicira.com
Fri Oct 16 04:35:27 UTC 2015


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.

> > +static void
> > +create_patch_ports(struct controller_ctx *ctx,
> > +                   const char *network,
> > +                   struct shash *existing_ports,
> > +                   const struct ovsrec_bridge *b1,
> > +                   const struct ovsrec_bridge *b2)
> 
> This could use a comment, since many of the arguments are non-obvious.
> In particular, how "existing_ports" will be modified and the
> difference between "b1" and "b2".

This gets a little more obvious later, in the same commit I mentioned
above, since it gets reduced to a trivial two-statement function.

> > +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?



More information about the dev mailing list