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

Justin Pettit jpettit at vmware.com
Fri Oct 16 08:15:19 UTC 2015


> 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.

--Justin





More information about the dev mailing list