> On Oct 15, 2015, at 9:35 PM, Ben Pfaff <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
