> On Sep 10, 2015, at 3:07 PM, Justin Pettit <jpet...@nicira.com> wrote: > > >> On Aug 25, 2015, at 1:03 PM, Bruce Davie <bda...@nicira.com> wrote: >> >> diff --git a/vtep/vtep.xml b/vtep/vtep.xml >> index ff8d0fe..a554dcf 100644 >> --- a/vtep/vtep.xml >> +++ b/vtep/vtep.xml >> @@ -367,7 +367,7 @@ >> <group title="BFD Local Configuration"> >> <p> >> The HSC writes the key-value pairs in the >> - <ref column="bfd_config_local"/> column to specifiy the local >> + <ref column="bfd_config_local"/> column to specify the local > > There are a few typo fixes in this patch. Normally, with a big change like > this patch, I wouldn't roll those in. The main reason is that if the patch > gets reverted at some point, these fixes would be lost, too. It's probably > not worth re-spinning this patch, but thought I'd mention it.
Yes, I realized that probably wasn’t ideal. I don’t mind splitting them out. > >> + <column name="acl_bindings"> >> + <p> >> + Attach Access Control Lists (ACLs) to the physical port. The >> + column consists of a map of VLAN tags to <ref table="ACL"/>s. If >> the value of >> + the VLAN tag in the map is 0, this means that the ACL is >> + associated with the entire physical port. Non-zero values mean >> + that the ACL is to be applied only on packets carrying that VLAN >> + tag value. Switches will not necessarily support matching on the >> + VLAN tag for all ACLs, and unsupported ACL bindings will cause >> + errors to be reported. >> + </p> > > This isn't related to this patch, but I didn't see what happens when > "vlan_bindings" or "vlan_stats" uses a key of 0. It seems that this was documented once but fell out of the schema at some point. I believe a value of zero indicates untagged traffic in those fields, and we should do a new patch to address that. As noted previously, I also intend to update the text quoted above to include a statement about not mixing zero and non-zero values on the same physical port. I don’t believe such a restriction needs to be applied in the places you mentioned. > >> @@ -851,6 +870,15 @@ >> One or more static routes, mapping IP prefixes to next hop IP addresses. >> </column> >> >> + <column name="acl_binding"> >> + Maps ACLs to logical router interfaces. The router interfaces >> + are indicated using IP address notation, and must be the same >> + interfaces created in the <ref column="switch_binding"/> >> + column. For example, an ACL could be associated with the logical >> + router interface with an address of 192.68.1.1 as defined in the >> + example above. >> + </column> > > The Physical_Port table has an "invalid_ACL_binding" error. Do you think > it's worth adding one to the Logical_Router table, too? Yes, that seems reasonable. > >> + <table name="ACL_entry"> >> + <p> >> + Describes the individual entries that comprise an Access Control List. >> + </p> >> + <p> >> + Each entry in the table is a single rule to match on certain >> + header fields. While there are a large number of fields that can >> + be matched on, most hardware cannot match on arbitrary >> + combinations of fields. It is common to match on either L2 >> + fields (described below in the L2 group of columns) or L3/L4 fields >> + (the L3/L4 group of columns) but not both. The hardware switch >> + controller may log an error if an ACL entry requires it to match >> + on an incompatible mixture of fields. > > Do you think it's worth mentioning that all packets are allowed by default? Sure. > >> + <column name="sequence"> >> + <p> >> + The sequence number for the ACL entry for the purpose of >> + ordering entries in an ACL. > > I think it would be helpful to indicate whether a larger number is more > likely to hit or less likely. > Yes. >> + </column> >> + <column name="ethertype"> >> + <p> >> + Ethertype in hexadecimal, in the form >> + <var>0xAAA</var> > > This is very minor, but I think it may be clearer with a fourth "A”. Agreed. > >> + <column name="source_port_min"> >> + <p> >> + Lower end of the range of source port values. >> + </p> >> + </column> >> + <column name="source_port_max"> >> + <p> >> + Upper end of the range of source port values. >> + </p> >> + </column> >> + <column name="dest_port_min"> >> + <p> >> + Lower end of the range of destination port values. >> + </p> >> + </column> >> + <column name="dest_port_max"> >> + <p> >> + Upper end of the range of destination port values. >> + </p> > > For all of these, it would be good to indicate if the chosen value is > included. (I assume it is.) > Agreed. >> + <column name="tcp_flags"> >> + <p> >> + Integer representing the value of TCP flags to match. >> + </p> >> + </column> >> + <column name="tcp_flags_mask"> >> + <p> >> + Integer representing the mask to apply when matching TCP flags. >> + </p> > > It might be nice to include an example for "tcp_flags" and its mask. > Yes, good suggestion. > Otherwise, it looks good. > OK, I’ll work on an update. Thanks Bruce > Thanks, > > --Justin > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
