> On Oct 9, 2015, at 9:20 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> index 90c72ff..f25709c 100644
> --- a/ovn/controller/patch.c
> +++ b/ovn/controller/patch.c
> @@ -50,7 +50,7 @@ match_patch_port(const struct ovsrec_port *port, const char
> *peer)
>
> static void
> create_patch_port(struct controller_ctx *ctx,
> - const char *network,
> + const char *key, const char *value,
> const struct ovsrec_bridge *src, const char *src_name,
> const struct ovsrec_bridge *dst, const char *dst_name,
> struct shash *existing_ports)
The names "key" and "value" seem awfully generic.
> static void
> create_patch_ports(struct controller_ctx *ctx,
> - const char *network,
> + const char *key, const char *value,
Same here about "key" and "value".
I wonder if this function is really needed. This only has one caller and it
doesn't do much. The other function that creates patch ports just calls
create_patch_port() directly.
> + char *src_name = xasprintf("patch-%s-to-%s", local, peer);
> + char *dst_name = xasprintf("patch-%s-to-%s", peer, local);
In the previous patch, you defined patch_port_name(), which could be used for
this.
> + create_patch_port(ctx, "ovn-logical-patch-port", local,
> + br_int, src_name, br_int, dst_name,
> + existing_ports);
A description of "ovn-localnet-port" is provided in the ovn-controller man
page. It might be nice to do the same for "ovn-logical-patch-port".
> /* Add any patch ports that should exist but don't. */
> parse_bridge_mappings(ctx, br_int, &existing_ports);
> + add_logical_patch_ports(ctx, br_int, &existing_ports);
Since these are doing very similar things, do you think it's worth using more
similar names?
> @@ -252,6 +262,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id
> mff_ovn_geneve,
> *
> * The "localnet" port may be configured with a VLAN ID. If so,
> * 'tag' will be set to that VLAN ID; otherwise 'tag' is 0.
> + *
Did you add this on purpose?
Acked-by: Justin Pettit <jpet...@nicira.com>
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
