diff mbox

[ovs-dev,1/3] flow: Add packet_size option to flow_compose.

Message ID 1500475921-4185-2-git-send-email-i.maximets@samsung.com
State Superseded
Headers show

Commit Message

Ilya Maximets July 19, 2017, 2:51 p.m. UTC 
This allows to compose packets with different real lenghts from
odp flows i.e. memory will be allocated for requested packet
size and all required headers like ip->tot_len filled correctly.

Will be used in netdev-dummy to properly handle '--len' option.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/flow.c                   | 65 ++++++++++++++++++++++++++++++--------------
 lib/flow.h                   |  2 +-
 lib/netdev-dummy.c           |  2 +-
 ofproto/ofproto-dpif-trace.c |  2 +-
 ofproto/ofproto-dpif.c       |  2 +-
 ovn/controller/ofctrl.c      |  2 +-
 tests/test-ovn.c             |  2 +-
 7 files changed, 51 insertions(+), 26 deletions(-)

Comments

Andy Zhou July 21, 2017, 10:38 p.m. UTC | #1 
On Wed, Jul 19, 2017 at 7:51 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
> This allows to compose packets with different real lenghts from
> odp flows i.e. memory will be allocated for requested packet
> size and all required headers like ip->tot_len filled correctly.
>
> Will be used in netdev-dummy to properly handle '--len' option.
>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Thank you for working on this.  Although those functions are mainly
for testing, it is still good that we improve them.

I am wondering about a slightly different approach. Instead of adding
'packet_size' to the flow_compose() interface, would it make
sense to come up with a new function whose task is to
expand a packet to the final length, (similar to flow_compose_l4_csum())

We would first create the necessary headers for all layers based on
flow, without fill in the actual size related field or compute checksums.

Then the fix size function will take over, fill in data, and
update various headers.

Then checksums can be computed and filled in.

I think the logics will be easier to follow with this approach. What
do you think?
diff mbox

Patch

diff --git a/lib/flow.c b/lib/flow.c
index e1597fa..fd0fac4 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -2613,16 +2613,14 @@  flow_set_mpls_lse(struct flow *flow, int idx, ovs_be32 lse)
 }
 
 static size_t
-flow_compose_l4(struct dp_packet *p, const struct flow *flow)
+flow_compose_l4(struct dp_packet *p, const struct flow *flow, size_t l4_len)
 {
-    size_t l4_len = 0;
-
     if (!(flow->nw_frag & FLOW_NW_FRAG_ANY)
         || !(flow->nw_frag & FLOW_NW_FRAG_LATER)) {
         if (flow->nw_proto == IPPROTO_TCP) {
             struct tcp_header *tcp;
 
-            l4_len = sizeof *tcp;
+            l4_len = MAX(l4_len, sizeof *tcp);
             tcp = dp_packet_put_zeros(p, l4_len);
             tcp->tcp_src = flow->tp_src;
             tcp->tcp_dst = flow->tp_dst;
@@ -2630,7 +2628,7 @@  flow_compose_l4(struct dp_packet *p, const struct flow *flow)
         } else if (flow->nw_proto == IPPROTO_UDP) {
             struct udp_header *udp;
 
-            l4_len = sizeof *udp;
+            l4_len = MAX(l4_len, sizeof *udp);
             udp = dp_packet_put_zeros(p, l4_len);
             udp->udp_src = flow->tp_src;
             udp->udp_dst = flow->tp_dst;
@@ -2638,30 +2636,31 @@  flow_compose_l4(struct dp_packet *p, const struct flow *flow)
         } else if (flow->nw_proto == IPPROTO_SCTP) {
             struct sctp_header *sctp;
 
-            l4_len = sizeof *sctp;
+            l4_len = MAX(l4_len, sizeof *sctp);
             sctp = dp_packet_put_zeros(p, l4_len);
             sctp->sctp_src = flow->tp_src;
             sctp->sctp_dst = flow->tp_dst;
         } else if (flow->nw_proto == IPPROTO_ICMP) {
             struct icmp_header *icmp;
 
-            l4_len = sizeof *icmp;
+            l4_len = MAX(l4_len, sizeof *icmp);
             icmp = dp_packet_put_zeros(p, l4_len);
             icmp->icmp_type = ntohs(flow->tp_src);
             icmp->icmp_code = ntohs(flow->tp_dst);
         } else if (flow->nw_proto == IPPROTO_IGMP) {
             struct igmp_header *igmp;
 
-            l4_len = sizeof *igmp;
+            l4_len = MAX(l4_len, sizeof *igmp);
             igmp = dp_packet_put_zeros(p, l4_len);
             igmp->igmp_type = ntohs(flow->tp_src);
             igmp->igmp_code = ntohs(flow->tp_dst);
             put_16aligned_be32(&igmp->group, flow->igmp_group_ip4);
         } else if (flow->nw_proto == IPPROTO_ICMPV6) {
             struct icmp6_hdr *icmp;
+            size_t icmpv6_len;
 
-            l4_len = sizeof *icmp;
-            icmp = dp_packet_put_zeros(p, l4_len);
+            icmpv6_len = sizeof *icmp;
+            icmp = dp_packet_put_zeros(p, icmpv6_len);
             icmp->icmp6_type = ntohs(flow->tp_src);
             icmp->icmp6_code = ntohs(flow->tp_dst);
 
@@ -2671,26 +2670,33 @@  flow_compose_l4(struct dp_packet *p, const struct flow *flow)
                 struct in6_addr *nd_target;
                 struct ovs_nd_lla_opt *lla_opt;
 
-                l4_len += sizeof *nd_target;
+                icmpv6_len += sizeof *nd_target;
                 nd_target = dp_packet_put_zeros(p, sizeof *nd_target);
                 *nd_target = flow->nd_target;
 
                 if (!eth_addr_is_zero(flow->arp_sha)) {
-                    l4_len += 8;
+                    icmpv6_len += 8;
                     lla_opt = dp_packet_put_zeros(p, 8);
                     lla_opt->len = 1;
                     lla_opt->type = ND_OPT_SOURCE_LINKADDR;
                     lla_opt->mac = flow->arp_sha;
                 }
                 if (!eth_addr_is_zero(flow->arp_tha)) {
-                    l4_len += 8;
+                    icmpv6_len += 8;
                     lla_opt = dp_packet_put_zeros(p, 8);
                     lla_opt->len = 1;
                     lla_opt->type = ND_OPT_TARGET_LINKADDR;
                     lla_opt->mac = flow->arp_tha;
                 }
             }
+            if (icmpv6_len < l4_len) {
+                dp_packet_put_zeros(p, l4_len - icmpv6_len);
+            } else {
+                l4_len = icmpv6_len;
+            }
         }
+    } else {
+         dp_packet_put_zeros(p, l4_len);
     }
     return l4_len;
 }
@@ -2741,20 +2747,28 @@  flow_compose_l4_csum(struct dp_packet *p, const struct flow *flow,
 }
 
 /* Puts into 'p' a packet that flow_extract() would parse as having the given
- * 'flow'.
+ * 'flow'.  Tries to create packet with size equal to 'packet_size'.  Resulted
+ * packet could be larger in case all required headers do not fit in the
+ * requested size.
  *
  * (This is useful only for testing, obviously, and the packet isn't really
  * valid.  Lots of fields are just zeroed.) */
 void
-flow_compose(struct dp_packet *p, const struct flow *flow)
+flow_compose(struct dp_packet *p, const struct flow *flow, size_t packet_size)
 {
     uint32_t pseudo_hdr_csum;
     size_t l4_len;
+    int extra_size;
 
     /* eth_compose() sets l3 pointer and makes sure it is 32-bit aligned. */
     eth_compose(p, flow->dl_dst, flow->dl_src, ntohs(flow->dl_type), 0);
     if (flow->dl_type == htons(FLOW_DL_TYPE_NONE)) {
         struct eth_header *eth = dp_packet_eth(p);
+
+        extra_size = packet_size - dp_packet_size(p);
+        if (extra_size > 0) {
+            dp_packet_put_zeros(p, extra_size);
+        }
         eth->eth_type = htons(dp_packet_size(p));
         return;
     }
@@ -2786,7 +2800,8 @@  flow_compose(struct dp_packet *p, const struct flow *flow)
 
         dp_packet_set_l4(p, dp_packet_tail(p));
 
-        l4_len = flow_compose_l4(p, flow);
+        extra_size = packet_size - dp_packet_size(p);
+        l4_len = flow_compose_l4(p, flow, (extra_size > 0) ? extra_size : 0);
 
         ip = dp_packet_l3(p);
         ip->ip_tot_len = htons(p->l4_ofs - p->l3_ofs + l4_len);
@@ -2795,6 +2810,9 @@  flow_compose(struct dp_packet *p, const struct flow *flow)
 
         pseudo_hdr_csum = packet_csum_pseudoheader(ip);
         flow_compose_l4_csum(p, flow, pseudo_hdr_csum);
+
+        return;
+
     } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
         struct ovs_16aligned_ip6_hdr *nh;
 
@@ -2809,13 +2827,17 @@  flow_compose(struct dp_packet *p, const struct flow *flow)
 
         dp_packet_set_l4(p, dp_packet_tail(p));
 
-        l4_len = flow_compose_l4(p, flow);
+        extra_size = packet_size - dp_packet_size(p);
+        l4_len = flow_compose_l4(p, flow, (extra_size > 0) ? extra_size : 0);
 
         nh = dp_packet_l3(p);
         nh->ip6_plen = htons(l4_len);
 
         pseudo_hdr_csum = packet_csum_pseudoheader6(nh);
         flow_compose_l4_csum(p, flow, pseudo_hdr_csum);
+
+        return;
+
     } else if (flow->dl_type == htons(ETH_TYPE_ARP) ||
                flow->dl_type == htons(ETH_TYPE_RARP)) {
         struct arp_eth_header *arp;
@@ -2835,9 +2857,7 @@  flow_compose(struct dp_packet *p, const struct flow *flow)
             arp->ar_sha = flow->arp_sha;
             arp->ar_tha = flow->arp_tha;
         }
-    }
-
-    if (eth_type_mpls(flow->dl_type)) {
+    } else if (eth_type_mpls(flow->dl_type)) {
         int n;
 
         p->l2_5_ofs = p->l3_ofs;
@@ -2850,6 +2870,11 @@  flow_compose(struct dp_packet *p, const struct flow *flow)
             push_mpls(p, flow->dl_type, flow->mpls_lse[--n]);
         }
     }
+
+    extra_size = packet_size - dp_packet_size(p);
+    if (extra_size > 0) {
+        dp_packet_put_zeros(p, extra_size);
+    }
 }
 
 /* Compressed flow. */
diff --git a/lib/flow.h b/lib/flow.h
index 9297842..651c0ef 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -124,7 +124,7 @@  void flow_set_mpls_tc(struct flow *, int idx, uint8_t tc);
 void flow_set_mpls_bos(struct flow *, int idx, uint8_t stack);
 void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse);
 
-void flow_compose(struct dp_packet *, const struct flow *);
+void flow_compose(struct dp_packet *, const struct flow *, size_t packet_size);
 
 bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto,
                          uint8_t *nw_frag);
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 51d29d5..6ee6a6a 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1478,7 +1478,7 @@  eth_from_flow(const char *s)
     }
 
     packet = dp_packet_new(0);
-    flow_compose(packet, &flow);
+    flow_compose(packet, &flow, 0);
 
     ofpbuf_uninit(&odp_key);
     return packet;
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index b3f3cbc..38d1100 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -376,7 +376,7 @@  parse_flow_and_packet(int argc, const char *argv[],
     /* Generate a packet, if requested. */
     if (packet) {
         if (!dp_packet_size(packet)) {
-            flow_compose(packet, flow);
+            flow_compose(packet, flow, 0);
         } else {
             /* Use the metadata from the flow and the packet argument
              * to reconstruct the flow. */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7cf1a40..eb4e493 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1287,7 +1287,7 @@  check_ct_eventmask(struct dpif_backer *backer)
 
     /* Compose a dummy UDP packet. */
     dp_packet_init(&packet, 0);
-    flow_compose(&packet, &flow);
+    flow_compose(&packet, &flow, 0);
 
     /* Execute the actions.  On older datapaths this fails with EINVAL, on
      * newer datapaths it succeeds. */
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 5aff230..7164ff0 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -1150,7 +1150,7 @@  ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char *flow_s,
     uint64_t packet_stub[128 / 8];
     struct dp_packet packet;
     dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
-    flow_compose(&packet, &uflow);
+    flow_compose(&packet, &uflow, 0);
 
     uint64_t ofpacts_stub[1024 / 8];
     struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index ca27a0f..a216b82 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -1164,7 +1164,7 @@  test_expr_to_packets(struct ovs_cmdl_context *ctx OVS_UNUSED)
         uint64_t packet_stub[128 / 8];
         struct dp_packet packet;
         dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
-        flow_compose(&packet, &uflow);
+        flow_compose(&packet, &uflow, 0);
 
         struct ds output = DS_EMPTY_INITIALIZER;
         const uint8_t *buf = dp_packet_data(&packet);