Hi Zonk Kai Li, Thanks for the review.
See inline for the comments.
On Mon, Jun 27, 2016 at 9:38 PM, Zong Kai LI <[email protected]> wrote:
> + <pre>
>> +eth.dst = eth.src;
>> +eth.src = <var>E</var>;
>> +ip4.dst = <var>O</var>;
>> +ip4.src = <var>S</var>;
>> +udp.src = 67;
>> +udp.dst = 68;
>> +outport = <var>P</var>;
>> +inport = ""; /* Allow sending out inport. */
>> +output;
>> + </pre>
>> +
>> + <p>
>> + where <var>E</var> is the server MAC address and <var>S</var>
>> is the
>> + server IPv4 address defined in the DHCPv4 options and
>> <var>O</var> is
>> + the IPv4 address defined in the logical port's addresses
>> column.
>> + </p>
>> +
>> + <p>
>> + (This terminates packet processing; the packet does not go on
>> the
>> + next ingress table.)
>> + </p>
>>
>
> Do you mean action "output;"? Yes, instead of going to the next ingress
> table, packets will be sent to table OFTABLE_REMOTE_OUTPUT, but "terminates
> packet processing" seems unclear or precise.
>
I will rephrase to "This terminates ingress packet processing".
>
> + </li>
>> +
>> + <li>
>> + A priority-0 flow that matches all packets to advances to table
>> 8.
>> + </li>
>> + </ul>
>> +
>> + <h3>Ingress Table 8: Destination Lookup</h3>
>>
>> <p>
>> This table implements switching behavior. It contains these
>> logical
>> @@ -387,6 +470,12 @@ output;
>> This is similar to ingress table 4 except for
>> <code>to-lport</code> ACLs.
>> </p>
>>
>> + <p>
>> + Also a priority 34000 logical flow is added for each logical port
>> which
>> + has DHCPv4 options defined to allow the DHCPv4 reply packet from
>> the
>> + <code>Ingress Table 7: DHCP response</code>.
>> + </p>
>>
>>
> 34000, a magic number ?
>
>
The ACL priority range is from 0 to 32767 which translates to OF priority
1000 to 33767 (see OVN_ACL_PRI_OFFSET in ovn-northd.c). Hence 34000, so
that the the CMS doesn't override this.
+static bool
>> has_stateful_acl(struct ovn_datapath *od)
>> {
>> for (size_t i = 0; i < od->nbs->n_acls; i++) {
>> @@ -1478,6 +1551,35 @@ build_acls(struct ovn_datapath *od, struct hmap
>> *lflows, struct hmap *ports)
>> acl->match, "drop;");
>> }
>> }
>> +
>> + /* Add 34000 priority flow to allow DHCP reply from ovn-controller
>> to all
>> + * logical ports of the datapath if the CMS has configured DHCPv4
>> options*/
>> + if (od->nbs && od->nbs->n_ports) {
>> + for (size_t i = 0; i < od->nbs->n_ports; i++) {
>> + if (od->nbs->ports[i]->dhcpv4_options) {
>> + const char *server_id = smap_get(
>> + &od->nbs->ports[i]->dhcpv4_options->options,
>> "server_id");
>> + const char *server_mac = smap_get(
>> + &od->nbs->ports[i]->dhcpv4_options->options,
>> "server_mac");
>> + const char *lease_time = smap_get(
>> + &od->nbs->ports[i]->dhcpv4_options->options,
>> "lease_time");
>> + const char *router = smap_get(
>> + &od->nbs->ports[i]->dhcpv4_options->options,
>> "router");
>> + if (server_id && server_mac && lease_time && router) {
>>
>
> Well, how about extract above code and write a new function to check if
> dhcpv4_options are valid ?
>
I thought about this, but since we need the values "server_mac" and
"server_id", I think the above code should be fine.
>
> static void
>> @@ -1635,7 +1737,71 @@ build_lswitch_flows(struct hmap *datapaths, struct
>> hmap *ports,
>> ovn_lflow_add(lflows, od, S_SWITCH_IN_ARP_RSP, 0, "1", "next;");
>> }
>>
>> - /* Ingress table 6: Destination lookup, broadcast and multicast
>> handling
>> + /* Logical switch ingress table 6 and 7: DHCP options and response
>> + * priority 100 flows. */
>> + HMAP_FOR_EACH (op, key_node, ports) {
>> + if (!op->nbs) {
>> + continue;
>> + }
>> +
>> + if (!lsp_is_enabled(op->nbs) || !strcmp(op->nbs->type,
>> "router")) {
>>
>
> Why not put op->nbs->dhcpv4_options checking here?
> And update to "... if the port is a router port or if dhcp is not enabled."
>
Ack. I will add this.
>
> + /* Don't add the DHCP flows if the port is not enabled or if
>> the
>> + * port is a router port. */
>> + continue;
>> + }
>> +
>> + for (size_t i = 0; i < op->nbs->n_addresses; i++) {
>> + struct lport_addresses laddrs;
>> + if (!extract_lsp_addresses(op->nbs->addresses[i], &laddrs,
>> + false)) {
>> + continue;
>> + }
>> +
>> + for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
>> + struct ds options_action = DS_EMPTY_INITIALIZER;
>> + struct ds response_action = DS_EMPTY_INITIALIZER;
>> + if (build_dhcpv4_action(op, laddrs.ipv4_addrs[j].addr,
>> + &options_action,
>> &response_action)) {
>>
>
> There are some dhcp_options relevant but not lsp address relevant checking
> in build_dhcpv4_action.
> So think about it, it's unnecessary to do duplicated check when lsp has
> multiple addresses.
>
> Can you please elaborate on this. I am not sure I understood correctly
> @@ -43,6 +43,11 @@
>> "max": "unlimited"}},
>> "up": {"type": {"key": "boolean", "min": 0, "max": 1}},
>> "enabled": {"type": {"key": "boolean", "min": 0, "max":
>> 1}},
>> + "dhcpv4_options": {"type": {"key": {"type": "uuid",
>> + "refTable": "DHCP_Options",
>> + "refType": "strong"},
>>
>
> Great for the refTable.
>
> That's all what I can see now.
>
> Thanks.
> Zong Kai, LI
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
