[ovs-dev] [PATCH 13/23] ovn: Implement logical patch ports.

[Justin Pettit]

> On Oct 9, 2015, at 9:20 PM, Ben Pfaff <blp at 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 <jpettit at nicira.com>

--Justin