On Fri, Oct 16, 2015 at 08:15:19AM +0000, Justin Pettit wrote:
>
> > 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.
OK, I made sure that the later version now has a comment about what
happens there.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
