On Thu, Jun 19, 2014 at 1:33 PM, Pravin Shelar <[email protected]> wrote:
> git am warning:
> /home/pravin/ovs/w7/.git/rebase-apply/patch:53: trailing whitespace.
>
> }
>
> warning: 1 line adds whitespace errors.
Fixed.
> -----------------------------------------------------------
> compiler warning:
>
> lib/odp-util.c:869:15: warning: cast from 'uint8_t *' (aka 'unsigned
> char *') to 'struct geneve_opt *'
> increases required alignment from 1 to 2 [-Wcast-align]
> opt = (struct geneve_opt *)((uint8_t *)opt + len);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/odp-util.c:845:63: warning: unused parameter 'tun' [-Wunused-parameter]
> parse_geneve_opts(const struct nlattr *attr, struct flow_tnl *tun)
Both fixed.
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index f1bb95d..7b108ed 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
>> @@ -455,6 +455,13 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port,
>> struct sw_flow_key *key)
>> struct ovs_tunnel_info *tun_info = OVS_CB(skb)->tun_info;
>> memcpy(&key->tun_key, &tun_info->tunnel,
>> sizeof(key->tun_key));
>> + if (tun_info->options) {
>> + memcpy(GENEVE_OPTS(key, tun_info->options_len),
>> + tun_info->options, tun_info->options_len);
> Need to check options_len before copying data from packet.
I think we should be OK here since we already checked it in
geneve_rcv() when we populated tun_info. Is there anything else that
we need to check?
>> diff --git a/datapath/vport-geneve.c b/datapath/vport-geneve.c
>> new file mode 100644
>> index 0000000..a6e9287
>> --- /dev/null
>> +++ b/datapath/vport-geneve.c
>> +static int geneve_rcv(struct sock *sk, struct sk_buff *skb)
>> +{
>> + struct geneve_port *geneve_port;
>> + struct genevehdr *geneveh;
>> + int opts_len;
>> + struct ovs_tunnel_info tun_info;
>> + __be64 key;
>> + __be16 flags;
>> +
>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,16,0)
>> + if (unlikely(udp_lib_checksum_complete(skb)))
>> + goto error;
>> +#endif
>> +
>> + if (unlikely(!pskb_may_pull(skb, GENEVE_BASE_HLEN)))
>> + goto error;
>> +
>> + geneveh = geneve_hdr(skb);
>> +
>> + if (unlikely(geneveh->ver != GENEVE_VER))
>> + goto error;
>> +
>> + if (unlikely(geneveh->proto_type != htons(ETH_P_TEB)))
>> + goto error;
>> +
>> + geneve_port = geneve_find_port(dev_net(skb->dev),
>> udp_hdr(skb)->dest);
>
> I guess we will start using rcu_dereference_sk_user_data() after
> uptreaming udp tunnels.
I think there's no reason why we can't start doing this now and it
will also help make this code look more like the existing upstream
code, so I went ahead and did that.
>> + if (unlikely(!geneve_port))
>> + goto error;
>> +
>> + opts_len = geneveh->opt_len * 4;
>> + if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len,
>> + htons(ETH_P_TEB)))
>> + goto error;
>> +
>> + geneveh = geneve_hdr(skb);
>> +
>> + flags = TUNNEL_KEY |
>> + (udp_hdr(skb)->check != 0 ? TUNNEL_CSUM : 0) |
>> + (geneveh->oam ? TUNNEL_OAM : 0);
>> +
> I am not sure why TUNNEL_CRIT_OPT is not parsed here.
This is mostly intended for devices that aren't capable of parsing the
options. Since OVS always parses options, we don't really have a use
for it. That being said, it's probably a good idea for consistency and
doesn't hurt anything.
>> +static int geneve_send(struct vport *vport, struct sk_buff *skb)
>> +{
>> + struct ovs_key_ipv4_tunnel *tun_key = &OVS_CB(skb)->tun_info->tunnel;
>> + int network_offset = skb_network_offset(skb);
>> + struct rtable *rt;
>> + int min_headroom;
>> + __be32 saddr;
>> + __be16 df;
>> + int sent_len;
>> + int err;
>> +
>> + if (unlikely(!OVS_CB(skb)->tun_info))
>> + return -EINVAL;
>> +
>> + /* Route lookup */
>> + saddr = tun_key->ipv4_src;
>> + rt = find_route(ovs_dp_get_net(vport->dp),
>> + &saddr, tun_key->ipv4_dst,
>> + IPPROTO_UDP, tun_key->ipv4_tos,
>> + skb->mark);
>> + if (IS_ERR(rt)) {
>> + err = PTR_ERR(rt);
>> + goto error;
>> + }
>> +
>> + min_headroom = LL_RESERVED_SPACE(rt_dst(rt).dev) +
>> rt_dst(rt).header_len
>> + + GENEVE_BASE_HLEN + sizeof(struct iphdr)
>> + + (vlan_tx_tag_present(skb) ? VLAN_HLEN : 0);
>> +
> we need to add options_len to headroom calculation.
Good catch.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
