On Tue, Nov 12, 2013 at 04:36:25PM +0200, Lorand Jakab wrote:
> This commit relaxes the assumption that all packets have an Ethernet
> header, and adds support for layer 3 flows. For each packet received on
> the Linux kernel datapath the l2 and l3 members of struct ofpbuf are
> intialized appropriately, and some functions now expect this (notable
> flow_extract()), in order to differentiate between layer 2 and layer 3
> packets. struct flow has now a new 'noeth' member, because we cannot
> assume that a flow has no Ethernet header when eth_src and eth_dst are
> 0. For layer 3 packets, the protocol type is still stored in the
> eth_type member.
>
> Switching L2->L3 and L3->L2 are both implemented by adding the pop_eth
> and push_eth actions respectively when a transition is detected. The
> push_eth action puts 0s on both source and destination MACs. These
> addresses can be modified with mod_dl_dst and mod_dl_src actions.
>
> Signed-off-by: Lorand Jakab <[email protected]>
This needs a rebase due to changes in dpif-netdev.
I get build assertion failures in flow.h. I build on 32-bit x86,
maybe you're using 64-bit:
../lib/flow.h:127:1: error: bit-field 'build_assert_failed' has negative width
(-1)
BUILD_ASSERT_DECL(offsetof(struct flow, nw_frag) + 1
^
It's probably a bad idea to put noeth at the very top of struct flow,
because it means that it is guaranteed to be followed by 3 or 7 bytes
(on 32 or 64 bit archs) of padding.
I'm not a fan of "negative" variable names, like "noeth", because it
makes logic harder to understand than necessary. Something like a
"base_layer" member that could be set to 2 or 3, or perhaps an enum
typed member, might be better.
The code doesn't entirely conform to OVS conventions, e.g. OVS always
puts {} around conditional statements.
Functions comments (e.g. flow_extract()) need updates to explain the
new conventions.
struct flow could use an expansion of its top-level comment to explain
the new conventions also.
The statement
+ packet->l2_5 = NULL;
is now in two places unnecessarily.
I don't think there's a good reason for the new 'goto' in
flow_extract(). I think an 'else' would work just as well.
Have you thought about interactions with OpenFlow yet? As-is, there
are going to be some oddities. For example, the flow
"dl_dst=00:00:00:00:00:00, actions=drop" will now drop all L3 packets,
I believe. Presumably we do not want that. I guess any flow that
matches on dl_dst or dl_src should implicitly also match noeth=false.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
