lib/ofpbuf: Remove 'l7' pointer.

Now that we don't need to parse TCP flags from the packet after
extraction, we usually do not need the 'l7' pointer any more.  When
needed, ofpbuf_get_tcp|udp|sctp|icmp_payload() or ofpbuf_get_l4_size()
can be used instead.

Removal of 'l7' was requested by Pravin for the DPDK datapath work, as
it simplifies packet parsing a bit.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

lib/bfd.c

@@ -654,6 +654,11 @@ bfd_process_packet(struct bfd *bfd, const struct flow *flow,
     enum flags flags;
     uint8_t version;
     struct msg *msg;
+    const uint8_t *l7 = ofpbuf_get_udp_payload(p);
+
+    if (!l7) {
+        return; /* No UDP payload. */
+    }
 
     /* This function is designed to follow section RFC 5880 6.8.6 closely. */
 
@@ -668,11 +673,11 @@ bfd_process_packet(struct bfd *bfd, const struct flow *flow,
         goto out;
     }
 
-    msg = ofpbuf_at(p, (uint8_t *)p->l7 - (uint8_t *)p->data, BFD_PACKET_LEN);
+    msg = ofpbuf_at(p, l7 - (uint8_t *)p->data, BFD_PACKET_LEN);
     if (!msg) {
         VLOG_INFO_RL(&rl, "%s: Received too-short BFD control message (only "
                      "%"PRIdPTR" bytes long, at least %d required).",
-                     bfd->name, (uint8_t *) ofpbuf_tail(p) - (uint8_t *) p->l7,
+                     bfd->name, (uint8_t *) ofpbuf_tail(p) - l7,
                      BFD_PACKET_LEN);
         goto out;
     }

lib/flow.c

@@ -69,31 +69,6 @@ pull_ip(struct ofpbuf *packet)
     return NULL;
 }
 
-static struct tcp_header *
-pull_tcp(struct ofpbuf *packet)
-{
-    if (packet->size >= TCP_HEADER_LEN) {
-        struct tcp_header *tcp = packet->data;
-        int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
-        if (tcp_len >= TCP_HEADER_LEN && packet->size >= tcp_len) {
-            return ofpbuf_pull(packet, tcp_len);
-        }
-    }
-    return NULL;
-}
-
-static struct udp_header *
-pull_udp(struct ofpbuf *packet)
-{
-    return ofpbuf_try_pull(packet, UDP_HEADER_LEN);
-}
-
-static struct sctp_header *
-pull_sctp(struct ofpbuf *packet)
-{
-    return ofpbuf_try_pull(packet, SCTP_HEADER_LEN);
-}
-
 static struct icmp_header *
 pull_icmp(struct ofpbuf *packet)
 {
@@ -258,46 +233,46 @@ parse_ipv6(struct ofpbuf *packet, struct flow *flow)
 }
 
 static void
-parse_tcp(struct ofpbuf *packet, struct ofpbuf *b, struct flow *flow)
+parse_tcp(struct ofpbuf *b, struct flow *flow)
 {
-    const struct tcp_header *tcp = pull_tcp(b);
+    if (b->size >= TCP_HEADER_LEN) {
-    if (tcp) {
+        const struct tcp_header *tcp = b->data;
+
         flow->tp_src = tcp->tcp_src;
         flow->tp_dst = tcp->tcp_dst;
         flow->tcp_flags = tcp->tcp_ctl & htons(0x0fff);
-        packet->l7 = b->data;
     }
 }
 
 static void
-parse_udp(struct ofpbuf *packet, struct ofpbuf *b, struct flow *flow)
+parse_udp(struct ofpbuf *b, struct flow *flow)
 {
-    const struct udp_header *udp = pull_udp(b);
+    if (b->size >= UDP_HEADER_LEN) {
-    if (udp) {
+        const struct udp_header *udp = b->data;
+
         flow->tp_src = udp->udp_src;
         flow->tp_dst = udp->udp_dst;
-        packet->l7 = b->data;
     }
 }
 
 static void
-parse_sctp(struct ofpbuf *packet, struct ofpbuf *b, struct flow *flow)
+parse_sctp(struct ofpbuf *b, struct flow *flow)
 {
-    const struct sctp_header *sctp = pull_sctp(b);
+    if (b->size >= SCTP_HEADER_LEN) {
-    if (sctp) {
+        const struct sctp_header *sctp = b->data;
+
         flow->tp_src = sctp->sctp_src;
         flow->tp_dst = sctp->sctp_dst;
-        packet->l7 = b->data;
     }
 }
 
-static bool
+static void
 parse_icmpv6(struct ofpbuf *b, struct flow *flow)
 {
     const struct icmp6_hdr *icmp = pull_icmpv6(b);
 
     if (!icmp) {
-        return false;
+        return;
     }
 
     /* The ICMPv6 type and code fields use the 16-bit transport port
@@ -312,7 +287,7 @@ parse_icmpv6(struct ofpbuf *b, struct flow *flow)
 
         nd_target = ofpbuf_try_pull(b, sizeof *nd_target);
         if (!nd_target) {
-            return false;
+            return;
         }
         flow->nd_target = *nd_target;
 
@@ -351,15 +326,14 @@ parse_icmpv6(struct ofpbuf *b, struct flow *flow)
         }
     }
 
-    return true;
+    return;
 
 invalid:
     memset(&flow->nd_target, 0, sizeof(flow->nd_target));
     memset(flow->arp_sha, 0, sizeof(flow->arp_sha));
     memset(flow->arp_tha, 0, sizeof(flow->arp_tha));
 
-    return false;
+    return;
-
 }
 
 /* Initializes 'flow' members from 'packet' and 'md'
@@ -376,9 +350,6 @@ invalid:
  *
  *    - packet->l4 to just past the IPv4 header, if one is present and has a
  *      correct length, and otherwise NULL.
- *
- *    - packet->l7 to just past the TCP/UDP/SCTP/ICMP header, if one is
- *      present and has a correct length, and otherwise NULL.
  */
 void
 flow_extract(struct ofpbuf *packet, const struct pkt_metadata *md,
@@ -402,7 +373,6 @@ flow_extract(struct ofpbuf *packet, const struct pkt_metadata *md,
     packet->l2_5 = NULL;
     packet->l3   = NULL;
     packet->l4   = NULL;
-    packet->l7   = NULL;
 
     if (b.size < sizeof *eth) {
         return;
@@ -448,17 +418,16 @@ flow_extract(struct ofpbuf *packet, const struct pkt_metadata *md,
 
             if (!(nh->ip_frag_off & htons(IP_FRAG_OFF_MASK))) {
                 if (flow->nw_proto == IPPROTO_TCP) {
-                    parse_tcp(packet, &b, flow);
+                    parse_tcp(&b, flow);
                 } else if (flow->nw_proto == IPPROTO_UDP) {
-                    parse_udp(packet, &b, flow);
+                    parse_udp(&b, flow);
                 } else if (flow->nw_proto == IPPROTO_SCTP) {
-                    parse_sctp(packet, &b, flow);
+                    parse_sctp(&b, flow);
                 } else if (flow->nw_proto == IPPROTO_ICMP) {
                     const struct icmp_header *icmp = pull_icmp(&b);
                     if (icmp) {
                         flow->tp_src = htons(icmp->icmp_type);
                         flow->tp_dst = htons(icmp->icmp_code);
-                        packet->l7 = b.data;
                     }
                 }
             }
@@ -470,15 +439,13 @@ flow_extract(struct ofpbuf *packet, const struct pkt_metadata *md,
 
         packet->l4 = b.data;
         if (flow->nw_proto == IPPROTO_TCP) {
-            parse_tcp(packet, &b, flow);
+            parse_tcp(&b, flow);
         } else if (flow->nw_proto == IPPROTO_UDP) {
-            parse_udp(packet, &b, flow);
+            parse_udp(&b, flow);
         } else if (flow->nw_proto == IPPROTO_SCTP) {
-            parse_sctp(packet, &b, flow);
+            parse_sctp(&b, flow);
         } else if (flow->nw_proto == IPPROTO_ICMPV6) {
-            if (parse_icmpv6(&b, flow)) {
+            parse_icmpv6(&b, flow);
-                packet->l7 = b.data;
-            }
         }
     } else if (flow->dl_type == htons(ETH_TYPE_ARP) ||
                flow->dl_type == htons(ETH_TYPE_RARP)) {
@@ -1270,7 +1237,7 @@ flow_set_mpls_lse(struct flow *flow, int idx, ovs_be32 lse)
     flow->mpls_lse[idx] = lse;
 }
 
-static void
+static uint8_t *
 flow_compose_l4(struct ofpbuf *b, const struct flow *flow)
 {
     if (!(flow->nw_frag & FLOW_NW_FRAG_ANY)
@@ -1282,21 +1249,21 @@ flow_compose_l4(struct ofpbuf *b, const struct flow *flow)
             tcp->tcp_src = flow->tp_src;
             tcp->tcp_dst = flow->tp_dst;
             tcp->tcp_ctl = TCP_CTL(ntohs(flow->tcp_flags), 5);
-            b->l7 = ofpbuf_tail(b);
+            return ofpbuf_tail(b);
         } else if (flow->nw_proto == IPPROTO_UDP) {
             struct udp_header *udp;
 
             udp = ofpbuf_put_zeros(b, sizeof *udp);
             udp->udp_src = flow->tp_src;
             udp->udp_dst = flow->tp_dst;
-            b->l7 = ofpbuf_tail(b);
+            return ofpbuf_tail(b);
         } else if (flow->nw_proto == IPPROTO_SCTP) {
             struct sctp_header *sctp;
 
             sctp = ofpbuf_put_zeros(b, sizeof *sctp);
             sctp->sctp_src = flow->tp_src;
             sctp->sctp_dst = flow->tp_dst;
-            b->l7 = ofpbuf_tail(b);
+            return ofpbuf_tail(b);
         } else if (flow->nw_proto == IPPROTO_ICMP) {
             struct icmp_header *icmp;
 
@@ -1304,7 +1271,7 @@ flow_compose_l4(struct ofpbuf *b, const struct flow *flow)
             icmp->icmp_type = ntohs(flow->tp_src);
             icmp->icmp_code = ntohs(flow->tp_dst);
             icmp->icmp_csum = csum(icmp, ICMP_HEADER_LEN);
-            b->l7 = ofpbuf_tail(b);
+            return ofpbuf_tail(b);
         } else if (flow->nw_proto == IPPROTO_ICMPV6) {
             struct icmp6_hdr *icmp;
 
@@ -1336,9 +1303,10 @@ flow_compose_l4(struct ofpbuf *b, const struct flow *flow)
             }
             icmp->icmp6_cksum = (OVS_FORCE uint16_t)
                 csum(icmp, (char *)ofpbuf_tail(b) - (char *)icmp);
-            b->l7 = ofpbuf_tail(b);
+            return ofpbuf_tail(b);
         }
     }
+    return NULL;
 }
 
 /* Puts into 'b' a packet that flow_extract() would parse as having the given
@@ -1389,6 +1357,7 @@ flow_compose(struct ofpbuf *b, const struct flow *flow)
         ip->ip_csum = csum(ip, sizeof *ip);
     } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
         struct ovs_16aligned_ip6_hdr *nh;
+        uint8_t *l7;
 
         nh = ofpbuf_put_zeros(b, sizeof *nh);
         put_16aligned_be32(&nh->ip6_flow, htonl(6 << 28) |
@@ -1401,10 +1370,9 @@ flow_compose(struct ofpbuf *b, const struct flow *flow)
 
         b->l4 = ofpbuf_tail(b);
 
-        flow_compose_l4(b, flow);
+        l7 = flow_compose_l4(b, flow);
 
-        nh->ip6_plen =
+        nh->ip6_plen = l7 ? htons(l7 - (uint8_t *) b->l4) : htons(0);
-            b->l7 ? htons((uint8_t *) b->l7 - (uint8_t *) b->l4) : htons(0);
     } else if (flow->dl_type == htons(ETH_TYPE_ARP) ||
                flow->dl_type == htons(ETH_TYPE_RARP)) {
         struct arp_eth_header *arp;

lib/ofp-print.c

@@ -62,25 +62,23 @@ ofp_packet_to_string(const void *data, size_t len)
     const struct pkt_metadata md = PKT_METADATA_INITIALIZER(0);
     struct ofpbuf buf;
     struct flow flow;
+    size_t l4_size;
 
     ofpbuf_use_const(&buf, data, len);
     flow_extract(&buf, &md, &flow);
     flow_format(&ds, &flow);
 
-    if (buf.l7) {
+    l4_size = ofpbuf_get_l4_size(&buf);
-        if (flow.nw_proto == IPPROTO_TCP) {
+
-            struct tcp_header *th = buf.l4;
+    if (flow.nw_proto == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) {
-            ds_put_format(&ds, " tcp_csum:%"PRIx16,
+        struct tcp_header *th = buf.l4;
-                          ntohs(th->tcp_csum));
+        ds_put_format(&ds, " tcp_csum:%"PRIx16, ntohs(th->tcp_csum));
-        } else if (flow.nw_proto == IPPROTO_UDP) {
+    } else if (flow.nw_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN) {
-            struct udp_header *uh = buf.l4;
+        struct udp_header *uh = buf.l4;
-            ds_put_format(&ds, " udp_csum:%"PRIx16,
+        ds_put_format(&ds, " udp_csum:%"PRIx16, ntohs(uh->udp_csum));
-                          ntohs(uh->udp_csum));
+    } else if (flow.nw_proto == IPPROTO_SCTP && l4_size >= SCTP_HEADER_LEN) {
-        } else if (flow.nw_proto == IPPROTO_SCTP) {
+        struct sctp_header *sh = buf.l4;
-            struct sctp_header *sh = buf.l4;
+        ds_put_format(&ds, " sctp_csum:%"PRIx32, ntohl(sh->sctp_csum));
-            ds_put_format(&ds, " sctp_csum:%"PRIx32,
-                          ntohl(sh->sctp_csum));
-        }
     }
 
     ds_put_char(&ds, '\n');

lib/ofpbuf.c

@@ -30,7 +30,7 @@ ofpbuf_use__(struct ofpbuf *b, void *base, size_t allocated,
     b->allocated = allocated;
     b->source = source;
     b->size = 0;
-    b->l2 = b->l2_5 = b->l3 = b->l4 = b->l7 = NULL;
+    b->l2 = b->l2_5 = b->l3 = b->l4 = NULL;
     list_poison(&b->list_node);
     b->private_p = NULL;
 }
@@ -182,9 +182,6 @@ ofpbuf_clone_with_headroom(const struct ofpbuf *buffer, size_t headroom)
     if (buffer->l4) {
         new_buffer->l4 = (char *) buffer->l4 + data_delta;
     }
-    if (buffer->l7) {
-        new_buffer->l7 = (char *) buffer->l7 + data_delta;
-    }
 
     return new_buffer;
 }
@@ -279,9 +276,6 @@ ofpbuf_resize__(struct ofpbuf *b, size_t new_headroom, size_t new_tailroom)
         if (b->l4) {
             b->l4 = (char *) b->l4 + data_delta;
         }
-        if (b->l7) {
-            b->l7 = (char *) b->l7 + data_delta;
-        }
     }
 }
 

lib/ofpbuf.h

@@ -20,6 +20,7 @@
 #include <stddef.h>
 #include <stdint.h>
 #include "list.h"
+#include "packets.h"
 #include "util.h"
 
 #ifdef  __cplusplus
@@ -48,7 +49,6 @@ struct ofpbuf {
     void *l2_5;                 /* MPLS label stack */
     void *l3;                   /* Network-level header. */
     void *l4;                   /* Transport-level header. */
-    void *l7;                   /* Application data. */
 
     struct list list_node;      /* Private list element for use by owner. */
     void *private_p;            /* Private pointer for use by owner. */
@@ -212,6 +212,43 @@ static inline bool ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b)
     return a->size == b->size && memcmp(a->data, b->data, a->size) == 0;
 }
 
+static inline size_t ofpbuf_get_l4_size(const struct ofpbuf *b)
+{
+    return b->l4 ? (const char *)ofpbuf_tail(b) - (const char *)b->l4 : 0;
+}
+
+static inline const void *ofpbuf_get_tcp_payload(const struct ofpbuf *b)
+{
+    size_t l4_size = ofpbuf_get_l4_size(b);
+
+    if (OVS_LIKELY(l4_size >= TCP_HEADER_LEN)) {
+        struct tcp_header *tcp = b->l4;
+        int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
+
+        if (OVS_LIKELY(tcp_len >= TCP_HEADER_LEN && tcp_len <= l4_size)) {
+            return (const char *)tcp + tcp_len;
+        }
+    }
+    return NULL;
+}
+
+static inline const void *ofpbuf_get_udp_payload(const struct ofpbuf *b)
+{
+    return OVS_LIKELY(ofpbuf_get_l4_size(b) >= UDP_HEADER_LEN)
+        ? (const char *)b->l4 + UDP_HEADER_LEN : NULL;
+}
+
+static inline const void *ofpbuf_get_sctp_payload(const struct ofpbuf *b)
+{
+    return OVS_LIKELY(ofpbuf_get_l4_size(b) >= SCTP_HEADER_LEN)
+        ? (const char *)b->l4 + SCTP_HEADER_LEN : NULL;
+}
+
+static inline const void *ofpbuf_get_icmp_payload(const struct ofpbuf *b)
+{
+    return OVS_LIKELY(ofpbuf_get_l4_size(b) >= ICMP_HEADER_LEN)
+        ? (const char *)b->l4 + ICMP_HEADER_LEN : NULL;
+}
 
 #ifdef  __cplusplus
 }

lib/packets.c

@@ -619,12 +619,13 @@ packet_set_ipv4_addr(struct ofpbuf *packet,
 {
     struct ip_header *nh = packet->l3;
     ovs_be32 old_addr = get_16aligned_be32(addr);
+    size_t l4_size = ofpbuf_get_l4_size(packet);
 
-    if (nh->ip_proto == IPPROTO_TCP && packet->l7) {
+    if (nh->ip_proto == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) {
         struct tcp_header *th = packet->l4;
 
         th->tcp_csum = recalc_csum32(th->tcp_csum, old_addr, new_addr);
-    } else if (nh->ip_proto == IPPROTO_UDP && packet->l7) {
+    } else if (nh->ip_proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN ) {
         struct udp_header *uh = packet->l4;
 
         if (uh->udp_csum) {
@@ -727,11 +728,13 @@ static void
 packet_update_csum128(struct ofpbuf *packet, uint8_t proto,
                      ovs_16aligned_be32 addr[4], const ovs_be32 new_addr[4])
 {
-    if (proto == IPPROTO_TCP && packet->l7) {
+    size_t l4_size = ofpbuf_get_l4_size(packet);
+
+    if (proto == IPPROTO_TCP && l4_size >= TCP_HEADER_LEN) {
         struct tcp_header *th = packet->l4;
 
         th->tcp_csum = recalc_csum128(th->tcp_csum, addr, new_addr);
-    } else if (proto == IPPROTO_UDP && packet->l7) {
+    } else if (proto == IPPROTO_UDP && l4_size >= UDP_HEADER_LEN) {
         struct udp_header *uh = packet->l4;
 
         if (uh->udp_csum) {

lib/pcap-file.c

@@ -303,15 +303,16 @@ tcp_reader_run(struct tcp_reader *r, const struct flow *flow,
     uint32_t hash;
     uint32_t seq;
     uint8_t flags;
+    const char *l7 = ofpbuf_get_tcp_payload(packet);
 
     if (flow->dl_type != htons(ETH_TYPE_IP)
         || flow->nw_proto != IPPROTO_TCP
-        || !packet->l7) {
+        || !l7) {
         return NULL;
     }
     tcp = packet->l4;
     flags = TCP_FLAGS(tcp->tcp_ctl);
-    l7_length = (char *) ofpbuf_tail(packet) - (char *) packet->l7;
+    l7_length = (char *) ofpbuf_tail(packet) - l7;
     seq = ntohl(get_16aligned_be32(&tcp->tcp_seq));
 
     /* Construct key. */
@@ -347,7 +348,7 @@ tcp_reader_run(struct tcp_reader *r, const struct flow *flow,
          * continually expanding it. */
         ofpbuf_shift(payload, (char *) payload->base - (char *) payload->data);
 
-        ofpbuf_put(payload, packet->l7, l7_length);
+        ofpbuf_put(payload, l7, l7_length);
         stream->seq_no += l7_length;
         return payload;
     } else {