>
> + <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.
+ </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 ?
> +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 ?
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."
+ /* 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.
@@ -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
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
