On Mon, Nov 10, 2014 at 12:46:15PM -0800, Pravin B Shelar wrote:
> Following patch adds support for userspace tunneling. Tunneling
> needs three more component first is routing table which is configured by
> caching kernel routes and second is ARP cache which build automatically
> by snooping arp. And third is tunnel protocol table which list all
> listening protocols which is populated by vswitchd as tunnel ports
> are added. GRE and VXLAN protocol support is added in this patch.
>
> Tunneling works as follows:
> On packet receive vswitchd check if this packet is targeted to tunnel
> port. If it is then vswitchd inserts tunnel pop action which pops
> header and sends packet to tunnel port.
> On packet xmit rather than generating Set tunnel action it generate
> tunnel push action which has tunnel header data. datapath can use
> tunnel-push action data to generate header for each packet and
> forward this packet to output port. Since tunnel-push action
> contains most of packet header vswitchd needs to lookup routing
> table and arp table to build this action.
>
> Signed-off-by: Pravin B Shelar <pshe...@nicira.com>
> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
> -
> Fixed according to comments from Jarno and Ben.
> Added test cases.
> Added documentation.
I spotted a few more nits. Thanks again for writing all this code.
In parse_gre_header, this:
if (greh->flags & ~(htons(GRE_CSUM) | htons(GRE_KEY) | htons(GRE_SEQ))) {
would be easier to read as:
if (greh->flags & ~htons(GRE_CSUM | GRE_KEY | GRE_SEQ)) {
In vxlan_extract_md(), this log message should probably ntohl() the
log arguments:
VLOG_WARN_RL(&err_rl, "invalid vxlan flags=%#x vni=%#x\n",
get_16aligned_be32(&vxh->vx_flags),
get_16aligned_be32(&vxh->vx_vni));
Also similarly here in format_odp_tnl_push_header():
ds_put_format(ds, "vxlan(flags=0x%"PRIx32",vni=0x%"PRIx32")",
get_16aligned_be32(&vxh->vx_flags),
get_16aligned_be32(&vxh->vx_vni));
and two other places later in the same function:
ds_put_format(ds, ",csum=0x%"PRIx32, get_16aligned_be32(options));
...
ds_put_format(ds, ",seq=0x%"PRIx32, get_16aligned_be32(options));
In format_odp_action(), these calls can be combined into one:
case OVS_ACTION_ATTR_TUNNEL_POP:
ds_put_format(ds, "tnl_pop(%"PRIu32, nl_attr_get_u32(a));
ds_put_cstr(ds, ")");
and actually it looks the first four ovs_scan_len() calls in
ovs_parse_tnl_push() could be just one, but maybe you think it looks
better as-is, which is fine with me.
Here in ovs_parse_tnl_push(), the doubled ( looks wrong:
} else if (ovs_scan_len(s, &n, "gre((flags=0x%"SCNx16",proto=0x%"SCNx16")",
although I see other doubled parentheses around so maybe it's
intentional?
There's an extra blank line here in ofproto/ofproto-dpif.c:
bool
ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
{
return ofproto_use_tnl_push_pop && ofproto->backer->enable_tnl_push_pop
&&
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
