On Wed, Jun 08, 2016 at 03:26:30PM +0800, Zong Kai LI wrote:
> This patch adds a new OVN action 'na' to support ND versus ARP.
>
> When ovn-controller received a ND packet, it frames a NA packet for
> reply, with mac address parsed from userdata as eth.dst. Then it
> reloads metadata info from previous packet to framed packet, and
> finally send the framed packet back with left actions.
>
> Eg. na{12:34:56:78:9a:bc; reg0 = 0x1; outport = inport; inport = ""; output;};
>
> Since patch port for IPv6 router interface is not ready yet, this
> patch will only try to deal with ND from VM. This patch will set
> RSO flags to 011 for NA packets.
>
> The next patch will do logical flows works for this action.
>
> Signed-off-by: Zong Kai LI <zealo...@gmail.com>
Thanks for the patch!
I think it would be better to retain more of the flavor of the arp
action here. For that action, it is the responsibility of whoever
writes the flow to initialize anything that cannot be inferred by the
action. For example, "arp" does not take a parameter that is assigned
to eth.src; instead, whoever writes the flow simply puts an assignment
to eth.src inside the nested set of actions. That's more flexible, too,
because it allows eth.src to come from someplace other than an immediate
constant.
The intent with OVN logical actions, by the way, is that only actions
should be enclosed in {}; data parameters should be enclosed in (). It
looks funny to me to mix both of them inside {}. But if we drop the
eth.src parameter then that solves the problem.
The documentation for reg0, outport, and inport seems strange, because
it implies that the action actually changes these. It doesn't, as far
as I can tell.
I still don't see any attempt to deal with alignment issues for RISC
machines. Using ALIGNED_CAST does not solve a problem, it only
suppresses a warning.
I'm appending the changes that I recommend for the documentation. My
changes do not actually implement these changes, only document them. My
changes also include some stylistic fixes to better match
CodingStyle.md.
Thanks,
Ben.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 1af9e89..79c4070 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -765,7 +765,7 @@ pinctrl_handle_na(const struct flow *ip_flow,
goto exit;
}
- // Frame the NA packet with RSO=011.
+ /* Frame the NA packet with RSO=011. */
uint64_t packet_stub[128 / 8];
struct dp_packet packet;
dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
@@ -774,7 +774,7 @@ pinctrl_handle_na(const struct flow *ip_flow,
&(ip_flow->nd_target), &(ip_flow->ipv6_src),
htons(0x6000));
- // Reload previous packet metadata.
+ /* Reload previous packet metadata. */
uint64_t ofpacts_stub[4096 / 8];
struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
reload_metadata(&ofpacts, md);
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 5189401..3fd626f 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -986,7 +986,7 @@
</dd>
<dt>
- <code>na{<var>A</var>; <var>action</var>; </code>...<code> };</code>
+ <code>na { <var>action</var>; </code>...<code> };</code>
</dt>
<dd>
@@ -999,38 +999,29 @@
<p>
The NA packet that this action operates on is initialized based on
- the IPv6 packet being processed(with userdata), as follows:
+ the IPv6 packet being processed, as follows. These are default
+ values that the nested actions will probably want to change:
</p>
<ul>
- <li><code>eth.dst</code> copied from eth.src</li>
- <li><code>eth.src</code> copied from userdata</li>
+ <li><code>eth.dst</code> copied from <code>eth.src</code>.</li>
+ <li><code>eth.src</code> unchanged</li>
<li><code>eth.type = 0x86dd</code></li>
<li><code>ip6.dst</code> copied from <code>ip6.src</code></li>
<li><code>ip6.src</code> copied from <code>nd.target</code></li>
<li><code>icmp6.type = 136</code> (Neighbor Advertisement)</li>
<li><code>nd.target</code> unchanged</li>
<li><code>nd.sll = 00:00:00:00:00:00</code></li>
- <li><code>nd.sll</code> copied from userdata</li>
+ <li><code>nd.sll</code> unchanged</li>
</ul>
<p>
- These are default values that the nested actions will probably want
- to change:
- <p>
-
- <ul>
- <li><code>reg0 = 0x1</code>(Mark as replied by ovn-controller)</li>
- <li><code>outport</code> copied from inport</li>
- <li><code>inport = ""</code></li>
- </ul>
-
The ND packet has the same VLAN header, if any, as the IP packet
it replaces.
</p>
<p>
- <b>Prerequisite:</b> <code>nd&&icmp6.type == 135</code>
+ <b>Prerequisite:</b> <code>nd && icmp6.type == 135</code>
</p>
</dd>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
