[ovs-dev] [PATCH 08/23] ovn-controller: Factor patch port management into new "patch" module.
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.
More information about the dev