On Tue, Apr 21, 2015 at 09:47:38AM -0400, Russell Bryant wrote:
> On 04/20/2015 06:29 PM, Ben Pfaff wrote:
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > v1->v2: Dropped remainder of series because it was committed.
> >
> > ovn/northd/automake.mk | 6 +-
> > ovn/northd/ovn-northd.c | 345
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > ovn/ovn-nb.xml | 14 +-
> > 3 files changed, 358 insertions(+), 7 deletions(-)
> >
>
> Only minor comments. This looks great to me.
>
> Acked-by: Russell Bryant <rbry...@redhat.com>
Thanks for the review.
> > + /* No such Pipeline row. Add one. */
> > + const struct sbrec_pipeline *pipeline;
> > + pipeline = sbrec_pipeline_insert(ctx->ovnsb_txn);
> > + sbrec_pipeline_set_logical_datapath(pipeline,
> > + logical_datapath->header_.uuid);
> > + sbrec_pipeline_set_table_id(pipeline, table_id);
> > + sbrec_pipeline_set_priority(pipeline, priority);
> > + sbrec_pipeline_set_match(pipeline, match);
> > + sbrec_pipeline_set_actions(pipeline, actions);
> > +
> > + VLOG_INFO("%s, %d, %d, %s, %s\n",
> > + logical_datapath->name, table_id, priority, match, actions);
>
> This may get a bit noisy. Maybe it should be just a debug message?
Oh, I thought I'd deleted that entirely. Evidently not! It's gone now.
> > +/* Appends port security constraints on L2 address field 'eth_addr_field'
> > + * (e.g. "eth.src" or "eth.dst") to 'match'. 'port_security', with
> > + * 'n_port_security' elements, is the collection of port_security
> > contraints
>
> typo on "constraints"
Thanks, fixed.
> > + /* Table 0: Admission control framework. */
> > + const struct nbrec_logical_switch *lswitch;
> > + NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > + /* Logical VLANs not supported. */
> > + pipeline_add(&pc, lswitch, 0, 100, "vlan.present", "drop");
>
> Child port traffic will have a VLAN tag. Do you want to make the
> updates for that use case later? I could take a stab at it.
That's a VLAN tag but it's not a *logical* VLAN tag; it's outside of the
logical network. Thus, no handling should be needed here, because
ovn-controller should transparently handle that for us.
> > index 6985f5e..a1b3a07 100644
> > --- a/ovn/ovn-nb.xml
> > +++ b/ovn/ovn-nb.xml
> > @@ -144,12 +144,14 @@
> > </p>
> >
> > <p>
> > - Exact syntax is TBD. One could simply use comma- or
> > - space-separated L2 and L3 addresses in each set member, or
> > - replace this by a subset of the general-purpose expression
> > - language used for the <ref column="match" table="Pipeline"
> > - db="OVN_Southbound"/> column in the OVN Southbound database's
> > - <ref table="Pipeline" db="OVN_Southbound"/> table.
> > + Each member of the set is a comma- or space-separated list. A single
> > + set member may have an Ethernet address, an IPv4 address, and an IPv6
> > + address, or any subset. Order is not significant.
> > + </p>
> > +
> > + <p>
> > + TBD: exact semantics. For now only Ethernet port security is
> > + implemented.
> > </p>
> > </column>
> >
> >
>
> You still have the tabs here instead of spaces for indentation.
Oops, fixed.
I'll apply this to ovn in a minute.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
