On Fri, 2017-04-21 at 00:21 -0700, Zhenyu Gao wrote:
> 1. Consume switch/case to judge type of frame instead of using if/else.
> 2. Add parse_ipv4hdr for ipv4 frame header parsing.
>
I think this patch addresses too many issues and should be broken up
into at least two different patches with appropriate subjects and commit
comments for each.
Thanks,
- Greg
> Signed-off-by: Zhenyu Gao <sysugaozhe...@gmail.com>
> ---
> datapath/flow.c | 230
> ++++++++++++++++++++++++++++----------------------------
> 1 file changed, 117 insertions(+), 113 deletions(-)
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0..0b35de6 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -250,6 +250,46 @@ static bool icmphdr_ok(struct sk_buff *skb)
> sizeof(struct icmphdr));
> }
>
> +/**
> + * Parse ipv4 header from an Ethernet frame.
> + * Return ipv4 header length if successful, otherwise a negative errno
> value.
> + */
> +static int parse_ipv4hdr(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + int err;
> + struct iphdr *nh;
> + __be16 offset;
> +
> + err = check_iphdr(skb);
> + if (unlikely(err))
> + return err;
> +
> + nh = ip_hdr(skb);
> + key->ipv4.addr.src = nh->saddr;
> + key->ipv4.addr.dst = nh->daddr;
> +
> + key->ip.proto = nh->protocol;
> + key->ip.tos = nh->tos;
> + key->ip.ttl = nh->ttl;
> +
> + offset = nh->frag_off & htons(IP_OFFSET);
> + if (offset) {
> + key->ip.frag = OVS_FRAG_TYPE_LATER;
> + } else {
> + if (nh->frag_off & htons(IP_MF) ||
> + skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + } else {
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
> + }
> + }
> + return ip_hdrlen(skb);
> +}
> +
> +/**
> + * Parse ipv6 header from an Ethernet frame.
> + * Return ipv6 header length if successful, otherwise a negative errno
> value.
> + */
> static int parse_ipv6hdr(struct sk_buff *skb, struct sw_flow_key *key)
> {
> unsigned int nh_ofs = skb_network_offset(skb);
> @@ -283,7 +323,10 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct
> sw_flow_key *key)
> else
> key->ip.frag = OVS_FRAG_TYPE_FIRST;
> } else {
> - key->ip.frag = OVS_FRAG_TYPE_NONE;
> + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> + key->ip.frag = OVS_FRAG_TYPE_FIRST;
> + else
> + key->ip.frag = OVS_FRAG_TYPE_NONE;
> }
>
> /* Delayed handling of error in ipv6_skip_exthdr() as it
> @@ -561,42 +604,43 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> key->eth.type = skb->protocol;
>
> /* Network layer. */
> - if (key->eth.type == htons(ETH_P_IP)) {
> - struct iphdr *nh;
> - __be16 offset;
> + switch(key->eth.type) {
> + case htons(ETH_P_IP):
> + case htons(ETH_P_IPV6): {
> + int nh_len;
> + if (key->eth.type == htons(ETH_P_IP)) {
> + nh_len = parse_ipv4hdr(skb, key);
> + } else {
> + nh_len = parse_ipv6hdr(skb, key);
> + }
>
> - error = check_iphdr(skb);
> - if (unlikely(error)) {
> - memset(&key->ip, 0, sizeof(key->ip));
> - memset(&key->ipv4, 0, sizeof(key->ipv4));
> - if (error == -EINVAL) {
> + if (unlikely(nh_len < 0)) {
> + switch (nh_len) {
> + case -EINVAL:
> + memset(&key->ip, 0, sizeof(key->ip));
> + if (key->eth.type == htons(ETH_P_IP)) {
> + memset(&key->ipv4.addr, 0,
> sizeof(key->ipv4.addr));
> + } else {
> + memset(&key->ipv6.addr, 0,
> sizeof(key->ipv6.addr));
> + }
> + /* fall-through */
> + case -EPROTO:
> skb->transport_header = skb->network_header;
> error = 0;
> + break;
> + default:
> + error = nh_len;
> }
> return error;
> }
>
> - nh = ip_hdr(skb);
> - key->ipv4.addr.src = nh->saddr;
> - key->ipv4.addr.dst = nh->daddr;
> -
> - key->ip.proto = nh->protocol;
> - key->ip.tos = nh->tos;
> - key->ip.ttl = nh->ttl;
> -
> - offset = nh->frag_off & htons(IP_OFFSET);
> - if (offset) {
> - key->ip.frag = OVS_FRAG_TYPE_LATER;
> + if (key->ip.frag == OVS_FRAG_TYPE_LATER) {
> return 0;
> }
> - if (nh->frag_off & htons(IP_MF) ||
> - skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> - key->ip.frag = OVS_FRAG_TYPE_FIRST;
> - else
> - key->ip.frag = OVS_FRAG_TYPE_NONE;
>
> /* Transport layer. */
> - if (key->ip.proto == IPPROTO_TCP) {
> + switch(key->ip.proto) {
> + case IPPROTO_TCP:
> if (tcphdr_ok(skb)) {
> struct tcphdr *tcp = tcp_hdr(skb);
> key->tp.src = tcp->source;
> @@ -605,8 +649,8 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> } else {
> memset(&key->tp, 0, sizeof(key->tp));
> }
> -
> - } else if (key->ip.proto == IPPROTO_UDP) {
> + break;
> + case IPPROTO_UDP:
> if (udphdr_ok(skb)) {
> struct udphdr *udp = udp_hdr(skb);
> key->tp.src = udp->source;
> @@ -614,7 +658,8 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> } else {
> memset(&key->tp, 0, sizeof(key->tp));
> }
> - } else if (key->ip.proto == IPPROTO_SCTP) {
> + break;
> + case IPPROTO_SCTP:
> if (sctphdr_ok(skb)) {
> struct sctphdr *sctp = sctp_hdr(skb);
> key->tp.src = sctp->source;
> @@ -622,7 +667,8 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> } else {
> memset(&key->tp, 0, sizeof(key->tp));
> }
> - } else if (key->ip.proto == IPPROTO_ICMP) {
> + break;
> + case IPPROTO_ICMP:
> if (icmphdr_ok(skb)) {
> struct icmphdr *icmp = icmp_hdr(skb);
> /* The ICMP type and code fields use the 16-bit
> @@ -634,10 +680,23 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> } else {
> memset(&key->tp, 0, sizeof(key->tp));
> }
> + break;
> + case NEXTHDR_ICMP:
> + if (icmp6hdr_ok(skb)) {
> + error = parse_icmpv6(skb, key, nh_len);
> + if (error)
> + return error;
> + } else {
> + memset(&key->tp, 0, sizeof(key->tp));
> + }
> + break;
> + default:
> + break;
> }
> -
> - } else if (key->eth.type == htons(ETH_P_ARP) ||
> - key->eth.type == htons(ETH_P_RARP)) {
> + break;
> + }
> + case htons(ETH_P_ARP):
> + case htons(ETH_P_RARP): {
> struct arp_eth_header *arp;
> bool arp_available = arphdr_ok(skb);
>
> @@ -663,94 +722,39 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> memset(&key->ip, 0, sizeof(key->ip));
> memset(&key->ipv4, 0, sizeof(key->ipv4));
> }
> - } else if (eth_p_mpls(key->eth.type)) {
> - size_t stack_len = MPLS_HLEN;
> -
> - /* In the presence of an MPLS label stack the end of the L2
> - * header and the beginning of the L3 header differ.
> - *
> - * Advance network_header to the beginning of the L3
> - * header. mac_len corresponds to the end of the L2 header.
> - */
> - while (1) {
> - __be32 lse;
> + break;
> + }
> + default:
> + if (eth_p_mpls(key->eth.type)) {
> + size_t stack_len = MPLS_HLEN;
> +
> + /* In the presence of an MPLS label stack the end of
> the L2
> + * header and the beginning of the L3 header differ.
> + *
> + * Advance network_header to the beginning of the L3
> + * header. mac_len corresponds to the end of the L2
> header.
> + */
> + while (1) {
> + __be32 lse;
>
> - error = check_header(skb, skb->mac_len + stack_len);
> - if (unlikely(error))
> - return 0;
> + error = check_header(skb, skb->mac_len +
> stack_len);
> + if (unlikely(error))
> + return 0;
>
> - memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
> + memcpy(&lse, skb_network_header(skb),
> MPLS_HLEN);
>
> - if (stack_len == MPLS_HLEN)
> - memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
> + if (stack_len == MPLS_HLEN)
> + memcpy(&key->mpls.top_lse, &lse,
> MPLS_HLEN);
>
> - skb_set_network_header(skb, skb->mac_len + stack_len);
> - if (lse & htonl(MPLS_LS_S_MASK))
> - break;
> + skb_set_network_header(skb, skb->mac_len +
> stack_len);
> + if (lse & htonl(MPLS_LS_S_MASK))
> + break;
>
> - stack_len += MPLS_HLEN;
> - }
> - } else if (key->eth.type == htons(ETH_P_IPV6)) {
> - int nh_len; /* IPv6 Header + Extensions */
> -
> - nh_len = parse_ipv6hdr(skb, key);
> - if (unlikely(nh_len < 0)) {
> - switch (nh_len) {
> - case -EINVAL:
> - memset(&key->ip, 0, sizeof(key->ip));
> - memset(&key->ipv6.addr, 0,
> sizeof(key->ipv6.addr));
> - /* fall-through */
> - case -EPROTO:
> - skb->transport_header = skb->network_header;
> - error = 0;
> - break;
> - default:
> - error = nh_len;
> - }
> - return error;
> - }
> -
> - if (key->ip.frag == OVS_FRAG_TYPE_LATER)
> - return 0;
> - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> - key->ip.frag = OVS_FRAG_TYPE_FIRST;
> -
> - /* Transport layer. */
> - if (key->ip.proto == NEXTHDR_TCP) {
> - if (tcphdr_ok(skb)) {
> - struct tcphdr *tcp = tcp_hdr(skb);
> - key->tp.src = tcp->source;
> - key->tp.dst = tcp->dest;
> - key->tp.flags = TCP_FLAGS_BE16(tcp);
> - } else {
> - memset(&key->tp, 0, sizeof(key->tp));
> - }
> - } else if (key->ip.proto == NEXTHDR_UDP) {
> - if (udphdr_ok(skb)) {
> - struct udphdr *udp = udp_hdr(skb);
> - key->tp.src = udp->source;
> - key->tp.dst = udp->dest;
> - } else {
> - memset(&key->tp, 0, sizeof(key->tp));
> - }
> - } else if (key->ip.proto == NEXTHDR_SCTP) {
> - if (sctphdr_ok(skb)) {
> - struct sctphdr *sctp = sctp_hdr(skb);
> - key->tp.src = sctp->source;
> - key->tp.dst = sctp->dest;
> - } else {
> - memset(&key->tp, 0, sizeof(key->tp));
> - }
> - } else if (key->ip.proto == NEXTHDR_ICMP) {
> - if (icmp6hdr_ok(skb)) {
> - error = parse_icmpv6(skb, key, nh_len);
> - if (error)
> - return error;
> - } else {
> - memset(&key->tp, 0, sizeof(key->tp));
> + stack_len += MPLS_HLEN;
> }
> }
> }
> +
> return 0;
> }
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
