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

[Ben Pfaff]

On Thu, Oct 15, 2015 at 05:48:30PM -0700, Justin Pettit wrote:
> 
> > 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.

They are in fact a key and a value in the external-ids column.

There's now a comment to explain:

/* Creates a patch port in bridge 'src' named 'src_name', whose peer is
 * 'dst_name' in bridge 'dst'.  Initializes the patch port's external-ids:'key'
 * to 'key'.
 *
 * If such a patch port already exists, removes it from 'existing_ports'. */

> > static void
> > create_patch_ports(struct controller_ctx *ctx,
> > -                   const char *network,
> > +                   const char *key, const char *value,
> 
> Same here about "key" and "value".

Ditto:

/* Creates a pair of patch ports that connect bridges 'b1' and 'b2', using a
 * port named 'name1' and 'name2' in each respective bridge.
 * external-ids:'key' in each port is initialized to 'value'.
 *
 * If one or both of the ports already exists, leaves it there and removes it
 * from 'existing_ports'. */

> 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.

I guess I see the obvious symmetry in the two lines of the function to
be a useful guide.

> > +            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.

The function couldn't be used exactly as written, since it takes two
bridges as parameters and uses the bridge names, but it's a reasonable
idea, so I changed it to take two string parameters instead.

> > +            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".

Good idea.  I added:

      <dt>
        <code>external-ids:ovn-logical-patch-port</code> in the
        <code>Port</code> table
      </dt>

      <dd>
        <p>
          This key identifies a patch port as one created by
          <code>ovn-controller</code> to implement an OVN logical patch port
          within the integration bridge.  Its value is the name of the OVN
          logical patch port that it implements.
        </p>
      </dd>

> >     /* 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?

I changes parse_bridge_mappings() to add_bridge_mappings(), in a
previous patch.

> > @@ -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?

No.  Reverted.

> Acked-by: Justin Pettit <jpettit at nicira.com>

Thanks!