On Thu, Oct 15, 2015 at 05:48:30PM -0700, Justin Pettit wrote:
>
> > 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.
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 <jpet...@nicira.com>
Thanks!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
