[PATCH] ip_frag: add IPv4 options fragment and unit test data

[PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 269 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ipv4_fragmentation.c |  52 ++++++-
 2 files changed, 301 insertions(+), 20 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..ecb9426 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,101 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+/* IP options */
+#define RTE_IPOPT_COPY				0x80
+#define RTE_IPOPT_CONTROL			0x00
+#define RTE_IPOPT_END				(0 | RTE_IPOPT_CONTROL)
+#define RTE_IPOPT_NOOP				(1 | RTE_IPOPT_CONTROL)
+#define RTE_IPOPT_COPIED(o)			((o) & RTE_IPOPT_COPY)
+#define RTE_IPOPT_MAX_LEN 40
+
+#define IPOPT_MANUAL
+
+#ifdef IPOPT_MANUAL
+uint8_t expected_first_frag_ipv4_opts[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts[] = {
+	RTE_IPOPT_NOOP, RTE_IPOPT_NOOP, RTE_IPOPT_NOOP, RTE_IPOPT_NOOP,
+	RTE_IPOPT_NOOP, RTE_IPOPT_NOOP, RTE_IPOPT_NOOP, RTE_IPOPT_NOOP,
+	RTE_IPOPT_NOOP, RTE_IPOPT_NOOP, RTE_IPOPT_NOOP, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+#else
+/**
+ * IPv4 Options
+ */
+struct test_ipv4_opt {
+	__extension__
+	union {
+		uint8_t type;		    /**< option type */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t number:5;   /**< option number */
+			uint8_t category:2; /**< option class */
+			uint8_t copied:1;   /**< option copy flag */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t copied:1;   /**< option copy flag */
+			uint8_t category:2; /**< option class */
+			uint8_t number:5;   /**< option number */
+#endif
+		} s_type;
+	};
+	uint8_t length;			    /**< option length */
+	uint8_t pointer;		    /**< option pointer */
+	uint8_t data[37];		    /**< option data */
+} __rte_packed;
+
+struct test_ipv4_opt test_ipv4_opts[] = {
+	{
+		.s_type.copied = 0,
+		.s_type.category = 0,
+		.s_type.number = 7,
+		.length = 11,
+		.pointer = 4,
+	},
+	{
+		.s_type.copied = 1,
+		.s_type.category = 0,
+		.s_type.number = 3,
+		.length = 7,
+		.pointer = 4,
+		.data[0] = 0xc0,
+		.data[1] = 0xa8,
+		.data[2] = 0xe3,
+		.data[3] = 0x96,
+	},
+};
+#endif
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPOPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +179,78 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag,
+	struct test_opt_data *expected_opt)
+{
+#ifdef IPOPT_MANUAL
+	if (is_first_frag) {
+		expected_opt->len = sizeof(expected_first_frag_ipv4_opts);
+		rte_memcpy(expected_opt->data, expected_first_frag_ipv4_opts,
+			sizeof(expected_first_frag_ipv4_opts));
+	} else {
+		expected_opt->len = sizeof(expected_sub_frag_ipv4_opts);
+		rte_memcpy(expected_opt->data, expected_sub_frag_ipv4_opts,
+			sizeof(expected_sub_frag_ipv4_opts));
+	}
+#else
+	uint16_t i;
+	uint16_t pos = 0;
+	expected_opt->len = 0;
+
+	for (i = 0; i < RTE_DIM(test_ipv4_opts); i++) {
+		if (unlikely(pos + test_ipv4_opts[i].length >
+				RTE_IPOPT_MAX_LEN))
+			return;
+
+		if (is_first_frag) {
+			rte_memcpy(expected_opt->data + pos, &test_ipv4_opts[i],
+				test_ipv4_opts[i].length);
+		} else {
+			if (test_ipv4_opts[i].s_type.copied)
+				rte_memcpy(expected_opt->data + pos,
+					&test_ipv4_opts[i],
+					test_ipv4_opts[i].length);
+			else
+				memset(expected_opt->data + pos, RTE_IPOPT_NOOP,
+					test_ipv4_opts[i].length);
+		}
+		expected_opt->len += test_ipv4_opts[i].length;
+		pos += test_ipv4_opts[i].length;
+	}
+
+	expected_opt->len = RTE_ALIGN_CEIL(expected_opt->len, 4);
+	memset(expected_opt->data + pos, RTE_IPOPT_END,
+		expected_opt->len - pos);
+#endif
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, &opt);
+
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +277,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +335,43 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (opt_len && (opt_len <= RTE_IPOPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPOPT_MAX_LEN;
+				memset(opt->data, RTE_IPOPT_END,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +391,43 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, 0},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, 0},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, 0},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
-		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
+		  {0x2000, 0x2046, 0x008C}, 1, 1},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, 1, 1},
+		 /* The middle fragment */
+		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          5,
+		  {0x200D, 0x2010, 0x2013, 0x2016, 0x2019}, 1, 0},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          5,
+		  {0x201A, 0x201D, 0x2020, 0x2023, 0x0026}, 1, 0},
 
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, 0},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, 0},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, 0},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +446,9 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +473,21 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len,
+					opt_res,
+					tests[i].ipv);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +496,35 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPOPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPOPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (%u)%s with (%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..bcafa29 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -1,4 +1,4 @@
-/* SPDX-License-Identifier: BSD-3-Clause
+/* SPDX-License-Identifier: (BSD-3-Clause OR GPL-2.0)
  * Copyright(c) 2010-2014 Intel Corporation
  */
 
@@ -12,6 +12,13 @@
 
 #include "ip_frag_common.h"
 
+/* IP options */
+#define RTE_IPOPT_COPY				0x80
+#define RTE_IPOPT_CONTROL			0x00
+#define RTE_IPOPT_END				(0 | RTE_IPOPT_CONTROL)
+#define RTE_IPOPT_NOOP				(1 | RTE_IPOPT_CONTROL)
+#define RTE_IPOPT_COPIED(o)			((o) & RTE_IPOPT_COPY)
+
 /* Fragment Offset */
 #define	RTE_IPV4_HDR_DF_SHIFT			14
 #define	RTE_IPV4_HDR_MF_SHIFT			13
@@ -41,6 +48,38 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+/*
+ *	Options "fragmenting", just fill options not
+ *	allowed in fragments with NOOPs.
+ *	Simple and stupid 8), but the most efficient way.
+ */
+static inline void ip_options_fragment(struct rte_ipv4_hdr *iph)
+{
+	unsigned char *optptr = (unsigned char *)iph +
+	    sizeof(struct rte_ipv4_hdr);
+	int l = (iph->version_ihl & RTE_IPV4_HDR_IHL_MASK) *
+	    RTE_IPV4_IHL_MULTIPLIER - sizeof(struct rte_ipv4_hdr);
+	int optlen;
+
+	while (l > 0) {
+		switch (*optptr) {
+		case RTE_IPOPT_END:
+			return;
+		case RTE_IPOPT_NOOP:
+			l--;
+			optptr++;
+			continue;
+		}
+		optlen = optptr[1];
+		if (optlen < 2 || optlen > l)
+			return;
+		if (!RTE_IPOPT_COPIED(*optptr))
+			memset(optptr, RTE_IPOPT_NOOP, optlen);
+		l -= optlen;
+		optptr += optlen;
+	}
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -188,6 +227,17 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
+		/*
+		 * ANK: dirty, but effective trick. Upgrade options only if
+		 * the segment to be fragmented was THE FIRST (otherwise,
+		 * options are already fixed) and make it ONCE
+		 * on the initial mbuf, so that all the following fragments
+		 * will inherit fixed options.
+		 */
+		if ((fragment_offset == 0) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))
+			ip_options_fragment(in_hdr);
+
 		fragment_offset = (uint16_t)(fragment_offset +
 		    out_pkt->pkt_len - header_len);
 
-- 
1.8.3.1

Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Dariusz Sosnowski]

On Wed, 24 Nov 2021 16:47:06 +0800, Huichao Cai wrote:
> +/*
> + *     Options "fragmenting", just fill options not
> + *     allowed in fragments with NOOPs.
> + *     Simple and stupid 8), but the most efficient way.
> + */
> +static inline void ip_options_fragment(struct rte_ipv4_hdr *iph)
> +{
> +       unsigned char *optptr = (unsigned char *)iph +
> +           sizeof(struct rte_ipv4_hdr);
> +       int l = (iph->version_ihl & RTE_IPV4_HDR_IHL_MASK) *
> +           RTE_IPV4_IHL_MULTIPLIER - sizeof(struct rte_ipv4_hdr);
> +       int optlen;
> +
> +       while (l > 0) {
> +               switch (*optptr) {
> +               case RTE_IPOPT_END:
> +                       return;
> +               case RTE_IPOPT_NOOP:
> +                       l--;
> +                       optptr++;
> +                       continue;
> +               }
> +               optlen = optptr[1];
> +               if (optlen < 2 || optlen > l)
> +                       return;
> +               if (!RTE_IPOPT_COPIED(*optptr))
> +                       memset(optptr, RTE_IPOPT_NOOP, optlen);
> +               l -= optlen;
> +               optptr += optlen;
> +       }
> +}
> +
I have a few concerns regarding this implementation:
- Any IPv4 option longer than 2 bytes with copied flag unset, will not be substituted by NOOP option. In effect it will be copied to all fragments.
- Substituting options with NOOP might cause rte_ipv4_fragment_packet to produce more fragments than necessary, since options with copied flag unset will still occupy space in IPv4 header.
It would require some benchmarking, but maybe a better solution would be to prepare a separate IPv4 header for fragments without unnecessary options. 

Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hi Dariusz


Substituting options with NOOP might cause rte_ipv4_fragment_packet to produce more fragments than necessary, since options with copied flag unset will still occupy space in IPv4 header.


--The "ip_options_fragment" just make a replacement and doesn't change the length of the IPv4 header.So I don't quite understand why it leads to produce more fragments.


but maybe a better solution would be to prepare a separate IPv4 header for fragments without unnecessary options.
--Yes, we can do this, but it adds some extra work, such as generating a new IPv4 header and reassembling the data,which has some performance implications.

Huichao Cai

[-- Attachment #2: Type: text/html, Size: 2297 bytes --]

[PATCH v2] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 292 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ipv4_fragmentation.c |  85 +++++++++-
 2 files changed, 355 insertions(+), 22 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..2f19790 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,106 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+/* IP options */
+#define RTE_IPOPT_EOL				0
+#define RTE_IPOPT_NOP				1
+#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
+#define RTE_IPOPT_MAX_LEN			40
+
+#define RTE_IPOPT_MANUAL
+
+#ifdef RTE_IPOPT_MANUAL
+uint8_t expected_first_frag_ipv4_opts[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+#ifdef RTE_IPOPT_KEEP_IP_HLEN
+uint8_t expected_sub_frag_ipv4_opts[] = {
+	RTE_IPOPT_NOP, RTE_IPOPT_NOP, RTE_IPOPT_NOP, RTE_IPOPT_NOP,
+	RTE_IPOPT_NOP, RTE_IPOPT_NOP, RTE_IPOPT_NOP, RTE_IPOPT_NOP,
+	RTE_IPOPT_NOP, RTE_IPOPT_NOP, RTE_IPOPT_NOP, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+#else
+uint8_t expected_sub_frag_ipv4_opts[] = {
+	0x83, 0x07, 0x04, 0xc0,
+	0xa8, 0xe3, 0x96, 0x00,
+};
+#endif
+#else
+/**
+ * IPv4 Options
+ */
+struct test_ipv4_opt {
+	__extension__
+	union {
+		uint8_t type;		    /**< option type */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t number:5;   /**< option number */
+			uint8_t category:2; /**< option class */
+			uint8_t copied:1;   /**< option copy flag */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t copied:1;   /**< option copy flag */
+			uint8_t category:2; /**< option class */
+			uint8_t number:5;   /**< option number */
+#endif
+		} s_type;
+	};
+	uint8_t length;			    /**< option length */
+	uint8_t pointer;		    /**< option pointer */
+	uint8_t data[37];		    /**< option data */
+} __rte_packed;
+
+struct test_ipv4_opt test_ipv4_opts[] = {
+	{
+		.s_type.copied = 0,
+		.s_type.category = 0,
+		.s_type.number = 7,
+		.length = 11,
+		.pointer = 4,
+	},
+	{
+		.s_type.copied = 1,
+		.s_type.category = 0,
+		.s_type.number = 3,
+		.length = 7,
+		.pointer = 4,
+		.data[0] = 0xc0,
+		.data[1] = 0xa8,
+		.data[2] = 0xe3,
+		.data[3] = 0x96,
+	},
+};
+#endif
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPOPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +184,85 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag,
+	struct test_opt_data *expected_opt)
+{
+#ifdef RTE_IPOPT_MANUAL
+	if (is_first_frag) {
+		expected_opt->len = sizeof(expected_first_frag_ipv4_opts);
+		rte_memcpy(expected_opt->data, expected_first_frag_ipv4_opts,
+			sizeof(expected_first_frag_ipv4_opts));
+	} else {
+		expected_opt->len = sizeof(expected_sub_frag_ipv4_opts);
+		rte_memcpy(expected_opt->data, expected_sub_frag_ipv4_opts,
+			sizeof(expected_sub_frag_ipv4_opts));
+	}
+#else
+	uint16_t i;
+	uint16_t pos = 0;
+	expected_opt->len = 0;
+
+	for (i = 0; i < RTE_DIM(test_ipv4_opts); i++) {
+		if (unlikely(pos + test_ipv4_opts[i].length >
+				RTE_IPOPT_MAX_LEN))
+			return;
+
+		if (is_first_frag) {
+			rte_memcpy(expected_opt->data + pos, &test_ipv4_opts[i],
+				test_ipv4_opts[i].length);
+			expected_opt->len += test_ipv4_opts[i].length;
+			pos += test_ipv4_opts[i].length;
+		} else {
+			if (test_ipv4_opts[i].s_type.copied) {
+				rte_memcpy(expected_opt->data + pos,
+					&test_ipv4_opts[i],
+					test_ipv4_opts[i].length);
+				expected_opt->len += test_ipv4_opts[i].length;
+				pos += test_ipv4_opts[i].length;
+#ifdef RTE_IPOPT_KEEP_IP_HLEN
+			} else {
+				memset(expected_opt->data + pos, RTE_IPOPT_NOP,
+					test_ipv4_opts[i].length);
+				expected_opt->len += test_ipv4_opts[i].length;
+				pos += test_ipv4_opts[i].length;
+#endif
+			}
+		}
+	}
+
+	expected_opt->len = RTE_ALIGN_CEIL(expected_opt->len, 4);
+	memset(expected_opt->data + pos, RTE_IPOPT_EOL,
+		expected_opt->len - pos);
+#endif
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, &opt);
+
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +289,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +347,43 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (opt_len && (opt_len <= RTE_IPOPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPOPT_MAX_LEN;
+				memset(opt->data, RTE_IPOPT_EOL,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +403,54 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, 0},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, 0},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, 0},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2046, 0x008C}, 1, 1},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, 1, 1},
+#ifdef RTE_IPOPT_KEEP_IP_HLEN
+		 /* The middle fragment */
+		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          5,
+		  {0x200D, 0x2010, 0x2013, 0x2016, 0x2019}, 1, 0},
+#else
+		 /* The middle fragment */
 		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
-
+		  {0x200D, 0x2012, 0x2017}, 1, 0},
+#endif
+#ifdef RTE_IPOPT_KEEP_IP_HLEN
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          5,
+		  {0x201A, 0x201D, 0x2020, 0x2023, 0x0026}, 1, 0},
+#else
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x201F, 0x0024}, 1, 0},
+#endif
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, 0},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, 0},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, 0},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +469,9 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +496,21 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len,
+					opt_res,
+					tests[i].ipv);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +519,35 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPOPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPOPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (%u)%s with (%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..82c070b 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -12,6 +12,12 @@
 
 #include "ip_frag_common.h"
 
+/* IP options */
+#define RTE_IPOPT_EOL				0
+#define RTE_IPOPT_NOP				1
+#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
+#define RTE_IPOPT_MAX_LEN			40
+
 /* Fragment Offset */
 #define	RTE_IPV4_HDR_DF_SHIFT			14
 #define	RTE_IPV4_HDR_MF_SHIFT			13
@@ -22,6 +28,8 @@
 
 #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
 
+#define	RTE_IPV4_HDR_MAX_LEN		60
+
 static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
 		const struct rte_ipv4_hdr *src, uint16_t header_len,
 		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
@@ -41,6 +49,58 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+static inline void __create_ipopt_frag_hdr(uint8_t *iph,
+	uint16_t *ipopt_len, uint8_t *ipopt_frag_hdr)
+{
+	uint16_t len = *ipopt_len;
+	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+
+	*ipopt_len = 0;
+	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+	iph_opt->ihl = sizeof(struct rte_ipv4_hdr) / RTE_IPV4_IHL_MULTIPLIER;
+	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
+
+	if (unlikely(len > RTE_IPOPT_MAX_LEN))
+		return;
+
+	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
+
+	while (len > 0) {
+		if (unlikely(*p_opt == RTE_IPOPT_NOP)) {
+			len--;
+			p_opt++;
+#ifdef RTE_IPOPT_KEEP_IP_HLEN
+			ipopt_frag_hdr[(*ipopt_len)++] = RTE_IPOPT_NOP;
+#endif
+			continue;
+		} else if (unlikely(*p_opt == RTE_IPOPT_EOL))
+			break;
+
+		if (p_opt[1] < 2 || p_opt[1] > len)
+			break;
+		if (RTE_IPOPT_COPIED(*p_opt)) {
+			rte_memcpy(ipopt_frag_hdr + *ipopt_len,
+				p_opt, p_opt[1]);
+			*ipopt_len += p_opt[1];
+#ifdef RTE_IPOPT_KEEP_IP_HLEN
+		} else {
+			memset(ipopt_frag_hdr + *ipopt_len,
+				RTE_IPOPT_NOP, p_opt[1]);
+			*ipopt_len += p_opt[1];
+#endif
+		}
+
+		len -= p_opt[1];
+		p_opt += p_opt[1];
+	}
+
+	len = RTE_ALIGN_CEIL(*ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
+	memset(ipopt_frag_hdr + *ipopt_len,
+		RTE_IPOPT_EOL, len - *ipopt_len);
+	*ipopt_len = len;
+	iph_opt->ihl += len / RTE_IPV4_IHL_MULTIPLIER;
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -76,6 +136,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	uint32_t more_in_segs;
 	uint16_t fragment_offset, flag_offset, frag_size, header_len;
 	uint16_t frag_bytes_remaining;
+	uint8_t ipopt_frag_hdr[RTE_IPV4_HDR_MAX_LEN];
+	uint16_t ipopt_len;
 
 	/*
 	 * Formal parameter checking.
@@ -117,6 +179,7 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	in_seg_data_pos = header_len;
 	out_pkt_pos = 0;
 	fragment_offset = 0;
+	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
 
 	more_in_segs = 1;
 	while (likely(more_in_segs)) {
@@ -188,10 +251,26 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
-		fragment_offset = (uint16_t)(fragment_offset +
-		    out_pkt->pkt_len - header_len);
+		/* Create a separate IP header to handle frag options. */
+		if (unlikely((fragment_offset == 0) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) &&
+			    (ipopt_len))) {
+			__create_ipopt_frag_hdr((uint8_t *)in_hdr,
+				&ipopt_len, ipopt_frag_hdr);
+
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
 
-		out_pkt->l3_len = header_len;
+			out_pkt->l3_len = header_len;
+
+			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
+			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+		} else {
+			fragment_offset = (uint16_t)(fragment_offset +
+			    out_pkt->pkt_len - header_len);
+
+			out_pkt->l3_len = header_len;
+		}
 
 		/* Write the fragment to the output list */
 		pkts_out[out_pkt_pos] = out_pkt;
-- 
1.8.3.1

RE: [PATCH v2] ip_frag: add IPv4 options fragment and test data

[Ananyev, Konstantin]

Hi Huichao,
 
> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 

...

> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index 2e7739d..82c070b 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -12,6 +12,12 @@
> 
>  #include "ip_frag_common.h"
> 
> +/* IP options */
> +#define RTE_IPOPT_EOL				0
> +#define RTE_IPOPT_NOP				1
> +#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
> +#define RTE_IPOPT_MAX_LEN			40
> +
>  /* Fragment Offset */
>  #define	RTE_IPV4_HDR_DF_SHIFT			14
>  #define	RTE_IPV4_HDR_MF_SHIFT			13
> @@ -22,6 +28,8 @@
> 
>  #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
> 
> +#define	RTE_IPV4_HDR_MAX_LEN		60
> +
>  static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
>  		const struct rte_ipv4_hdr *src, uint16_t header_len,
>  		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> @@ -41,6 +49,58 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		rte_pktmbuf_free(mb[i]);
>  }
> 
> +static inline void __create_ipopt_frag_hdr(uint8_t *iph,
> +	uint16_t *ipopt_len, uint8_t *ipopt_frag_hdr)
> +{
> +	uint16_t len = *ipopt_len;
> +	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +
> +	*ipopt_len = 0;
> +	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> +	iph_opt->ihl = sizeof(struct rte_ipv4_hdr) / RTE_IPV4_IHL_MULTIPLIER;
> +	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
> +
> +	if (unlikely(len > RTE_IPOPT_MAX_LEN))
> +		return;
> +
> +	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
> +
> +	while (len > 0) {
> +		if (unlikely(*p_opt == RTE_IPOPT_NOP)) {
> +			len--;
> +			p_opt++;
> +#ifdef RTE_IPOPT_KEEP_IP_HLEN

Who will define this macro and when?
In general we trying to avoid conditional compilations within DPDK.
Can we always use one way or another?
As you are doing a copy anyway, probably no harm just
completely remove RTE_IPOPT_KEEP_IP_HLEN and related behaviour
and copy only options that need to be copied.
WDYT?


> +			ipopt_frag_hdr[(*ipopt_len)++] = RTE_IPOPT_NOP;
> +#endif
> +			continue;
> +		} else if (unlikely(*p_opt == RTE_IPOPT_EOL))
> +			break;
> +
> +		if (p_opt[1] < 2 || p_opt[1] > len)
> +			break;
> +		if (RTE_IPOPT_COPIED(*p_opt)) {
> +			rte_memcpy(ipopt_frag_hdr + *ipopt_len,
> +				p_opt, p_opt[1]);
> +			*ipopt_len += p_opt[1];
> +#ifdef RTE_IPOPT_KEEP_IP_HLEN
> +		} else {
> +			memset(ipopt_frag_hdr + *ipopt_len,
> +				RTE_IPOPT_NOP, p_opt[1]);
> +			*ipopt_len += p_opt[1];
> +#endif
> +		}
> +
> +		len -= p_opt[1];
> +		p_opt += p_opt[1];
> +	}
> +
> +	len = RTE_ALIGN_CEIL(*ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
> +	memset(ipopt_frag_hdr + *ipopt_len,
> +		RTE_IPOPT_EOL, len - *ipopt_len);
> +	*ipopt_len = len;
> +	iph_opt->ihl += len / RTE_IPV4_IHL_MULTIPLIER;
> +}
> +
>  /**
>   * IPv4 fragmentation.
>   *
> @@ -76,6 +136,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  	uint32_t more_in_segs;
>  	uint16_t fragment_offset, flag_offset, frag_size, header_len;
>  	uint16_t frag_bytes_remaining;
> +	uint8_t ipopt_frag_hdr[RTE_IPV4_HDR_MAX_LEN];
> +	uint16_t ipopt_len;
> 
>  	/*
>  	 * Formal parameter checking.
> @@ -117,6 +179,7 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  	in_seg_data_pos = header_len;
>  	out_pkt_pos = 0;
>  	fragment_offset = 0;
> +	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
> 
>  	more_in_segs = 1;
>  	while (likely(more_in_segs)) {
> @@ -188,10 +251,26 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		    (uint16_t)out_pkt->pkt_len,
>  		    flag_offset, fragment_offset, more_in_segs);
> 
> -		fragment_offset = (uint16_t)(fragment_offset +
> -		    out_pkt->pkt_len - header_len);
> +		/* Create a separate IP header to handle frag options. */
> +		if (unlikely((fragment_offset == 0) &&
> +			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) &&
> +			    (ipopt_len))) {
> +			__create_ipopt_frag_hdr((uint8_t *)in_hdr,
> +				&ipopt_len, ipopt_frag_hdr);
> +
> +			fragment_offset = (uint16_t)(fragment_offset +
> +				out_pkt->pkt_len - header_len);
> 
> -		out_pkt->l3_len = header_len;
> +			out_pkt->l3_len = header_len;
> +
> +			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
> +			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +		} else {
> +			fragment_offset = (uint16_t)(fragment_offset +
> +			    out_pkt->pkt_len - header_len);
> +
> +			out_pkt->l3_len = header_len;
> +		}
> 
>  		/* Write the fragment to the output list */
>  		pkts_out[out_pkt_pos] = out_pkt;
> --
> 1.8.3.1

Re:RE: [PATCH v2] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

Hi Konstantin,


>Who will define this macro and when?
>In general we trying to avoid conditional compilations within DPDK.
>Can we always use one way or another?
>As you are doing a copy anyway, probably no harm just
>completely remove RTE_IPOPT_KEEP_IP_HLEN and related behaviour
>and copy only options that need to be copied.
>WDYT?
Yes.I agree with you.I define this macro because I'm not quite sure which way is better, so I want to hear from you.
I will completely remove RTE_IPOPT_KEEP_IP_HLEN and related behaviour and copy only options that need to be copied.

[-- Attachment #2: Type: text/html, Size: 1061 bytes --]

[PATCH v3] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 263 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ipv4_fragmentation.c |  77 +++++++++-
 2 files changed, 318 insertions(+), 22 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..996130d 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,96 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+/* IP options */
+#define RTE_IPOPT_EOL				0
+#define RTE_IPOPT_NOP				1
+#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
+#define RTE_IPOPT_MAX_LEN			40
+
+#define RTE_IPOPT_MANUAL
+
+#ifdef RTE_IPOPT_MANUAL
+uint8_t expected_first_frag_ipv4_opts[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts[] = {
+	0x83, 0x07, 0x04, 0xc0,
+	0xa8, 0xe3, 0x96, 0x00,
+};
+#else
+/**
+ * IPv4 Options
+ */
+struct test_ipv4_opt {
+	__extension__
+	union {
+		uint8_t type;		    /**< option type */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t number:5;   /**< option number */
+			uint8_t category:2; /**< option class */
+			uint8_t copied:1;   /**< option copy flag */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t copied:1;   /**< option copy flag */
+			uint8_t category:2; /**< option class */
+			uint8_t number:5;   /**< option number */
+#endif
+		} s_type;
+	};
+	uint8_t length;			    /**< option length */
+	uint8_t pointer;		    /**< option pointer */
+	uint8_t data[37];		    /**< option data */
+} __rte_packed;
+
+struct test_ipv4_opt test_ipv4_opts[] = {
+	{
+		.s_type.copied = 0,
+		.s_type.category = 0,
+		.s_type.number = 7,
+		.length = 11,
+		.pointer = 4,
+	},
+	{
+		.s_type.copied = 1,
+		.s_type.category = 0,
+		.s_type.number = 3,
+		.length = 7,
+		.pointer = 4,
+		.data[0] = 0xc0,
+		.data[1] = 0xa8,
+		.data[2] = 0xe3,
+		.data[3] = 0x96,
+	},
+};
+#endif
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPOPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +174,78 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag,
+	struct test_opt_data *expected_opt)
+{
+#ifdef RTE_IPOPT_MANUAL
+	if (is_first_frag) {
+		expected_opt->len = sizeof(expected_first_frag_ipv4_opts);
+		rte_memcpy(expected_opt->data, expected_first_frag_ipv4_opts,
+			sizeof(expected_first_frag_ipv4_opts));
+	} else {
+		expected_opt->len = sizeof(expected_sub_frag_ipv4_opts);
+		rte_memcpy(expected_opt->data, expected_sub_frag_ipv4_opts,
+			sizeof(expected_sub_frag_ipv4_opts));
+	}
+#else
+	uint16_t i;
+	uint16_t pos = 0;
+	expected_opt->len = 0;
+
+	for (i = 0; i < RTE_DIM(test_ipv4_opts); i++) {
+		if (unlikely(pos + test_ipv4_opts[i].length >
+				RTE_IPOPT_MAX_LEN))
+			return;
+
+		if (is_first_frag) {
+			rte_memcpy(expected_opt->data + pos, &test_ipv4_opts[i],
+				test_ipv4_opts[i].length);
+			expected_opt->len += test_ipv4_opts[i].length;
+			pos += test_ipv4_opts[i].length;
+		} else {
+			if (test_ipv4_opts[i].s_type.copied) {
+				rte_memcpy(expected_opt->data + pos,
+					&test_ipv4_opts[i],
+					test_ipv4_opts[i].length);
+				expected_opt->len += test_ipv4_opts[i].length;
+				pos += test_ipv4_opts[i].length;
+			}
+		}
+	}
+
+	expected_opt->len = RTE_ALIGN_CEIL(expected_opt->len, 4);
+	memset(expected_opt->data + pos, RTE_IPOPT_EOL,
+		expected_opt->len - pos);
+#endif
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, &opt);
+
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +272,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +330,43 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (opt_len && (opt_len <= RTE_IPOPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPOPT_MAX_LEN;
+				memset(opt->data, RTE_IPOPT_EOL,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +386,42 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, 0},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, 0},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, 0},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2046, 0x008C}, 1, 1},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, 1, 1},
+		 /* The middle fragment */
 		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
-
+		  {0x200D, 0x2012, 0x2017}, 1, 0},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x201F, 0x0024}, 1, 0},
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, 0},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, 0},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, 0},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +440,9 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +467,21 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len,
+					opt_res,
+					tests[i].ipv);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +490,35 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPOPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPOPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (%u)%s with (%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..57b8bc1 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -12,6 +12,12 @@
 
 #include "ip_frag_common.h"
 
+/* IP options */
+#define RTE_IPOPT_EOL				0
+#define RTE_IPOPT_NOP				1
+#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
+#define RTE_IPOPT_MAX_LEN			40
+
 /* Fragment Offset */
 #define	RTE_IPV4_HDR_DF_SHIFT			14
 #define	RTE_IPV4_HDR_MF_SHIFT			13
@@ -22,6 +28,8 @@
 
 #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
 
+#define	RTE_IPV4_HDR_MAX_LEN		60
+
 static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
 		const struct rte_ipv4_hdr *src, uint16_t header_len,
 		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
@@ -41,6 +49,50 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+static inline void __create_ipopt_frag_hdr(uint8_t *iph,
+	uint16_t *ipopt_len, uint8_t *ipopt_frag_hdr)
+{
+	uint16_t len = *ipopt_len;
+	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+
+	*ipopt_len = 0;
+	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+	iph_opt->ihl = sizeof(struct rte_ipv4_hdr) / RTE_IPV4_IHL_MULTIPLIER;
+	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
+
+	if (unlikely(len > RTE_IPOPT_MAX_LEN))
+		return;
+
+	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
+
+	while (len > 0) {
+		if (unlikely(*p_opt == RTE_IPOPT_NOP)) {
+			len--;
+			p_opt++;
+			continue;
+		} else if (unlikely(*p_opt == RTE_IPOPT_EOL))
+			break;
+
+		if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
+			break;
+
+		if (RTE_IPOPT_COPIED(*p_opt)) {
+			rte_memcpy(ipopt_frag_hdr + *ipopt_len,
+				p_opt, p_opt[1]);
+			*ipopt_len += p_opt[1];
+		}
+
+		len -= p_opt[1];
+		p_opt += p_opt[1];
+	}
+
+	len = RTE_ALIGN_CEIL(*ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
+	memset(ipopt_frag_hdr + *ipopt_len,
+		RTE_IPOPT_EOL, len - *ipopt_len);
+	*ipopt_len = len;
+	iph_opt->ihl += len / RTE_IPV4_IHL_MULTIPLIER;
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -76,6 +128,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	uint32_t more_in_segs;
 	uint16_t fragment_offset, flag_offset, frag_size, header_len;
 	uint16_t frag_bytes_remaining;
+	uint8_t ipopt_frag_hdr[RTE_IPV4_HDR_MAX_LEN];
+	uint16_t ipopt_len;
 
 	/*
 	 * Formal parameter checking.
@@ -117,6 +171,7 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	in_seg_data_pos = header_len;
 	out_pkt_pos = 0;
 	fragment_offset = 0;
+	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
 
 	more_in_segs = 1;
 	while (likely(more_in_segs)) {
@@ -188,10 +243,26 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
-		fragment_offset = (uint16_t)(fragment_offset +
-		    out_pkt->pkt_len - header_len);
+		/* Create a separate IP header to handle frag options. */
+		if (unlikely((fragment_offset == 0) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) &&
+			    (ipopt_len))) {
+			__create_ipopt_frag_hdr((uint8_t *)in_hdr,
+				&ipopt_len, ipopt_frag_hdr);
+
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
 
-		out_pkt->l3_len = header_len;
+			out_pkt->l3_len = header_len;
+
+			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
+			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+		} else {
+			fragment_offset = (uint16_t)(fragment_offset +
+			    out_pkt->pkt_len - header_len);
+
+			out_pkt->l3_len = header_len;
+		}
 
 		/* Write the fragment to the output list */
 		pkts_out[out_pkt_pos] = out_pkt;
-- 
1.8.3.1

RE: [PATCH v3] ip_frag: add IPv4 options fragment and test data

[Ananyev, Konstantin]

Ho Huichao,

> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.

Apologies for delay in getting back to you.
LGTM in general, just few extra questions/nits/suggestions below.

> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---
>  app/test/test_ipfrag.c               | 263 ++++++++++++++++++++++++++++++++---
>  lib/ip_frag/rte_ipv4_fragmentation.c |  77 +++++++++-
>  2 files changed, 318 insertions(+), 22 deletions(-)
> 
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> index 1ced25a..996130d 100644
> --- a/app/test/test_ipfrag.c
> +++ b/app/test/test_ipfrag.c
> @@ -18,10 +18,96 @@
>  #define NUM_MBUFS 128
>  #define BURST 32
> 
> +/* IP options */
> +#define RTE_IPOPT_EOL				0
> +#define RTE_IPOPT_NOP				1
> +#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
> +#define RTE_IPOPT_MAX_LEN			40

These macros are dups for what we have in rte_ipv4_fragmentation.c
Would probably make sense to name them RTE_IPV4_IPOPT_... and put them 
in some public .h to avoid duplication.

> +#define RTE_IPOPT_MANUAL

Could you clarify what this macro does?
BTW, I assume it is a local one?
If so, no need for RTE_ prefix.

> +#ifdef RTE_IPOPT_MANUAL
> +uint8_t expected_first_frag_ipv4_opts[] = {
> +	0x07, 0x0b, 0x04, 0x00,
> +	0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x83,
> +	0x07, 0x04, 0xc0, 0xa8,
> +	0xe3, 0x96, 0x00, 0x00,
> +};
> +
> +uint8_t expected_sub_frag_ipv4_opts[] = {
> +	0x83, 0x07, 0x04, 0xc0,
> +	0xa8, 0xe3, 0x96, 0x00,
> +};
> +#else
> +/**
> + * IPv4 Options
> + */
> +struct test_ipv4_opt {
> +	__extension__
> +	union {
> +		uint8_t type;		    /**< option type */
> +		struct {
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> +			uint8_t number:5;   /**< option number */
> +			uint8_t category:2; /**< option class */
> +			uint8_t copied:1;   /**< option copy flag */
> +#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> +			uint8_t copied:1;   /**< option copy flag */
> +			uint8_t category:2; /**< option class */
> +			uint8_t number:5;   /**< option number */
> +#endif
> +		} s_type;
> +	};
> +	uint8_t length;			    /**< option length */
> +	uint8_t pointer;		    /**< option pointer */
> +	uint8_t data[37];		    /**< option data */
> +} __rte_packed;
> +
> +struct test_ipv4_opt test_ipv4_opts[] = {
> +	{
> +		.s_type.copied = 0,
> +		.s_type.category = 0,
> +		.s_type.number = 7,
> +		.length = 11,
> +		.pointer = 4,
> +	},
> +	{
> +		.s_type.copied = 1,
> +		.s_type.category = 0,
> +		.s_type.number = 3,
> +		.length = 7,
> +		.pointer = 4,
> +		.data[0] = 0xc0,
> +		.data[1] = 0xa8,
> +		.data[2] = 0xe3,
> +		.data[3] = 0x96,
> +	},
> +};
> +#endif
> +
> +struct test_opt_data {
> +	bool is_first_frag;		 /**< offset is 0 */
> +	uint16_t len;			 /**< option data len */
> +	uint8_t data[RTE_IPOPT_MAX_LEN]; /**< option data */
> +};
> +
>  static struct rte_mempool *pkt_pool,
>  			  *direct_pool,
>  			  *indirect_pool;
> 
> +static inline void
> +hex_to_str(uint8_t *hex, uint16_t len, char *str)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++) {
> +		sprintf(str, "%02x", hex[i]);
> +		str += 2;
> +	}
> +	*str = 0;
> +}
> +
>  static int
>  setup_buf_pool(void)
>  {
> @@ -88,23 +174,78 @@ static void ut_teardown(void)
>  {
>  }
> 
> +static inline void
> +test_get_ipv4_opt(bool is_first_frag,
> +	struct test_opt_data *expected_opt)
> +{
> +#ifdef RTE_IPOPT_MANUAL
> +	if (is_first_frag) {
> +		expected_opt->len = sizeof(expected_first_frag_ipv4_opts);
> +		rte_memcpy(expected_opt->data, expected_first_frag_ipv4_opts,
> +			sizeof(expected_first_frag_ipv4_opts));
> +	} else {
> +		expected_opt->len = sizeof(expected_sub_frag_ipv4_opts);
> +		rte_memcpy(expected_opt->data, expected_sub_frag_ipv4_opts,
> +			sizeof(expected_sub_frag_ipv4_opts));
> +	}
> +#else
> +	uint16_t i;
> +	uint16_t pos = 0;
> +	expected_opt->len = 0;
> +
> +	for (i = 0; i < RTE_DIM(test_ipv4_opts); i++) {
> +		if (unlikely(pos + test_ipv4_opts[i].length >
> +				RTE_IPOPT_MAX_LEN))
> +			return;
> +
> +		if (is_first_frag) {
> +			rte_memcpy(expected_opt->data + pos, &test_ipv4_opts[i],
> +				test_ipv4_opts[i].length);
> +			expected_opt->len += test_ipv4_opts[i].length;
> +			pos += test_ipv4_opts[i].length;
> +		} else {
> +			if (test_ipv4_opts[i].s_type.copied) {
> +				rte_memcpy(expected_opt->data + pos,
> +					&test_ipv4_opts[i],
> +					test_ipv4_opts[i].length);
> +				expected_opt->len += test_ipv4_opts[i].length;
> +				pos += test_ipv4_opts[i].length;
> +			}
> +		}
> +	}
> +
> +	expected_opt->len = RTE_ALIGN_CEIL(expected_opt->len, 4);
> +	memset(expected_opt->data + pos, RTE_IPOPT_EOL,
> +		expected_opt->len - pos);
> +#endif
> +}
> +
>  static void
> -v4_allocate_packet_of(struct rte_mbuf *b, int fill,
> -		      size_t s, int df, uint8_t mf, uint16_t off,
> -		      uint8_t ttl, uint8_t proto, uint16_t pktid)
> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
> +	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
> +	uint16_t pktid, bool have_opt, bool is_first_frag)
>  {
>  	/* Create a packet, 2k bytes long */
>  	b->data_off = 0;
>  	char *data = rte_pktmbuf_mtod(b, char *);
> -	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
> +	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
> +	uint16_t iph_len;
> +	struct test_opt_data opt;
> 
> -	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
> +	opt.len = 0;
> +
> +	if (have_opt)
> +		test_get_ipv4_opt(is_first_frag, &opt);
> +
> +	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
> +	memset(data, fill, iph_len + s);
> 
>  	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
> 
> -	hdr->version_ihl = 0x45; /* standard IP header... */
> +	hdr->version_ihl = 0x40; /* ipv4 */
> +	hdr->version_ihl += (iph_len / 4);
>  	hdr->type_of_service = 0;
> -	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
> +	b->pkt_len = s + iph_len;
>  	b->data_len = b->pkt_len;
>  	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
>  	hdr->packet_id = rte_cpu_to_be_16(pktid);
> @@ -131,6 +272,8 @@ static void ut_teardown(void)
>  	hdr->hdr_checksum = 0;
>  	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
>  	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
> +
> +	rte_memcpy(hdr + 1, opt.data, opt.len);
>  }
> 
>  static void
> @@ -187,6 +330,43 @@ static void ut_teardown(void)
>  	}
>  }
> 
> +static inline void
> +test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
> +	struct test_opt_data *opt, int ipv)
> +{
> +	int32_t i;
> +
> +	for (i = 0; i < num; i++) {
> +		if (ipv == 4) {
> +			struct rte_ipv4_hdr *iph =
> +			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
> +			uint16_t header_len = (iph->version_ihl &
> +				RTE_IPV4_HDR_IHL_MASK) *
> +				RTE_IPV4_IHL_MULTIPLIER;
> +			uint16_t opt_len = header_len -
> +				sizeof(struct rte_ipv4_hdr);
> +
> +			if ((rte_be_to_cpu_16(iph->fragment_offset) &
> +				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
> +				opt->is_first_frag = true;
> +			else
> +				opt->is_first_frag = false;
> +
> +			if (opt_len && (opt_len <= RTE_IPOPT_MAX_LEN)) {
> +				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
> +				    char *, sizeof(struct rte_ipv4_hdr));
> +				opt->len = opt_len;
> +				rte_memcpy(opt->data, iph_opt, opt_len);
> +			} else {
> +				opt->len = RTE_IPOPT_MAX_LEN;
> +				memset(opt->data, RTE_IPOPT_EOL,
> +				    sizeof(opt->data));
> +			}
> +			opt++;
> +		}
> +	}
> +}
> +
>  static int
>  test_ip_frag(void)
>  {
> @@ -206,32 +386,42 @@ static void ut_teardown(void)
>  		uint16_t pkt_id;
>  		int      expected_frags;
>  		uint16_t expected_fragment_offset[BURST];
> +		bool have_opt;
> +		bool is_first_frag;
>  	} tests[] = {
>  		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> -		  {0x2000, 0x009D}},
> +		  {0x2000, 0x009D}, 0},
>  		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
> -		  {0x2000, 0x009D}},
> +		  {0x2000, 0x009D}, 0},
>  		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
> -		  {0x2000, 0x2048, 0x0090}},
> +		  {0x2000, 0x2048, 0x0090}, 0},
>  		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
>  		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
>  		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
> -		  {0x2000, 0x2048, 0x0090}},
> +		  {0x2000, 0x2046, 0x008C}, 1, 1},
> +		 /* The first fragment */
> +		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
> +		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, 1, 1},
> +		 /* The middle fragment */
>  		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
> -		  {0x200D, 0x2013, 0x2019}},
> -
> +		  {0x200D, 0x2012, 0x2017}, 1, 0},
> +		 /* The last fragment */
> +		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
> +		  {0x201A, 0x201F, 0x0024}, 1, 0},
>  		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> -		  {0x0001, 0x04D0}},
> +		  {0x0001, 0x04D0}, 0},
>  		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> -		  {0x0001, 0x04E0}},
> +		  {0x0001, 0x04E0}, 0},
>  		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
>  		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
> -		  {0x0001, 0x04E0}},
> +		  {0x0001, 0x04E0}, 0},
>  	};
> 
>  	for (i = 0; i < RTE_DIM(tests); i++) {
>  		int32_t len = 0;
>  		uint16_t fragment_offset[BURST];
> +		struct test_opt_data opt_res[BURST];
> +		struct test_opt_data opt_exp;
>  		uint16_t pktid = tests[i].pkt_id;
>  		struct rte_mbuf *pkts_out[BURST];
>  		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> @@ -250,7 +440,9 @@ static void ut_teardown(void)
>  					      tests[i].set_of,
>  					      tests[i].ttl,
>  					      tests[i].proto,
> -					      pktid);
> +					      pktid,
> +					      tests[i].have_opt,
> +					      tests[i].is_first_frag);
>  		} else if (tests[i].ipv == 6) {
>  			v6_allocate_packet_of(b, 0x41414141,
>  					      tests[i].pkt_size,
> @@ -275,17 +467,21 @@ static void ut_teardown(void)
>  		if (len > 0) {
>  			test_get_offset(pkts_out, len,
>  			    fragment_offset, tests[i].ipv);
> +			if (tests[i].have_opt)
> +				test_get_frag_opt(pkts_out, len,
> +					opt_res,
> +					tests[i].ipv);
>  			test_free_fragments(pkts_out, len);
>  		}
> 
> -		printf("%zd: checking %d with %d\n", i, len,
> +		printf("[check frag number]%zd: checking %d with %d\n", i, len,
>  		       tests[i].expected_frags);
>  		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
>  				      "Failed case %zd.\n", i);
> 
>  		if (len > 0) {
>  			for (j = 0; j < (size_t)len; j++) {
> -				printf("%zd-%zd: checking %d with %d\n",
> +				printf("[check offset]%zd-%zd: checking %d with %d\n",
>  				    i, j, fragment_offset[j],
>  				    rte_cpu_to_be_16(
>  					tests[i].expected_fragment_offset[j]));
> @@ -294,6 +490,35 @@ static void ut_teardown(void)
>  					tests[i].expected_fragment_offset[j]),
>  				    "Failed case %zd.\n", i);
>  			}
> +
> +			if (tests[i].have_opt && (tests[i].ipv == 4)) {
> +				for (j = 0; j < (size_t)len; j++) {
> +					char opt_res_str[2 *
> +						RTE_IPOPT_MAX_LEN + 1];
> +					char opt_exp_str[2 *
> +						RTE_IPOPT_MAX_LEN + 1];
> +
> +					test_get_ipv4_opt(
> +						opt_res[j].is_first_frag,
> +						&opt_exp);
> +					hex_to_str(opt_res[j].data,
> +						opt_res[j].len,
> +						opt_res_str);
> +					hex_to_str(opt_exp.data,
> +						opt_exp.len,
> +						opt_exp_str);
> +
> +					printf(
> +						"[check ipv4 option]%zd-%zd: checking (%u)%s with (%u)%s\n",
> +						i, j,
> +						opt_res[j].len, opt_res_str,
> +						opt_exp.len, opt_exp_str);
> +						RTE_TEST_ASSERT_SUCCESS(
> +							strcmp(opt_res_str,
> +								opt_exp_str),
> +						"Failed case %zd.\n", i);
> +				}
> +			}
>  		}
> 
>  	}
> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index 2e7739d..57b8bc1 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -12,6 +12,12 @@
> 
>  #include "ip_frag_common.h"
> 
> +/* IP options */
> +#define RTE_IPOPT_EOL				0
> +#define RTE_IPOPT_NOP				1
> +#define RTE_IPOPT_COPIED(v)			((v) & 0x80)
> +#define RTE_IPOPT_MAX_LEN			40
> +
>  /* Fragment Offset */
>  #define	RTE_IPV4_HDR_DF_SHIFT			14
>  #define	RTE_IPV4_HDR_MF_SHIFT			13
> @@ -22,6 +28,8 @@
> 
>  #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
> 
> +#define	RTE_IPV4_HDR_MAX_LEN		60
> +
>  static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
>  		const struct rte_ipv4_hdr *src, uint16_t header_len,
>  		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> @@ -41,6 +49,50 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		rte_pktmbuf_free(mb[i]);
>  }
> 
> +static inline void __create_ipopt_frag_hdr(uint8_t *iph,
> +	uint16_t *ipopt_len, uint8_t *ipopt_frag_hdr)
> +{


Instead of returning void and having out parameter (ipopt_len),
why just not make it a return value?
static inline uint16_t
__create_ipopt_frag_hdr(const uint8_t *iph, uint8_t * ipopt_frag_hdr, uint16_t len)
{
	....
	return ipopt_len;
}

> +	uint16_t len = *ipopt_len;
> +	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +
> +	*ipopt_len = 0;
> +	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> +	iph_opt->ihl = sizeof(struct rte_ipv4_hdr) / RTE_IPV4_IHL_MULTIPLIER;

We probably can update ihl once at the very end of this function.

> +	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
> +
> +	if (unlikely(len > RTE_IPOPT_MAX_LEN))
> +		return;
> +
> +	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
> +
> +	while (len > 0) {
> +		if (unlikely(*p_opt == RTE_IPOPT_NOP)) {
> +			len--;
> +			p_opt++;
> +			continue;
> +		} else if (unlikely(*p_opt == RTE_IPOPT_EOL))
> +			break;
> +
> +		if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
> +			break;
> +
> +		if (RTE_IPOPT_COPIED(*p_opt)) {
> +			rte_memcpy(ipopt_frag_hdr + *ipopt_len,
> +				p_opt, p_opt[1]);
> +			*ipopt_len += p_opt[1];
> +		}
> +
> +		len -= p_opt[1];
> +		p_opt += p_opt[1];
> +	}
> +
> +	len = RTE_ALIGN_CEIL(*ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
> +	memset(ipopt_frag_hdr + *ipopt_len,
> +		RTE_IPOPT_EOL, len - *ipopt_len);
> +	*ipopt_len = len;
> +	iph_opt->ihl += len / RTE_IPV4_IHL_MULTIPLIER;
> +}
> +
>  /**
>   * IPv4 fragmentation.
>   *
> @@ -76,6 +128,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  	uint32_t more_in_segs;
>  	uint16_t fragment_offset, flag_offset, frag_size, header_len;
>  	uint16_t frag_bytes_remaining;
> +	uint8_t ipopt_frag_hdr[RTE_IPV4_HDR_MAX_LEN];
> +	uint16_t ipopt_len;
> 
>  	/*
>  	 * Formal parameter checking.
> @@ -117,6 +171,7 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  	in_seg_data_pos = header_len;
>  	out_pkt_pos = 0;
>  	fragment_offset = 0;
> +	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
> 
>  	more_in_segs = 1;
>  	while (likely(more_in_segs)) {
> @@ -188,10 +243,26 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		    (uint16_t)out_pkt->pkt_len,
>  		    flag_offset, fragment_offset, more_in_segs);
> 
> -		fragment_offset = (uint16_t)(fragment_offset +
> -		    out_pkt->pkt_len - header_len);
> +		/* Create a separate IP header to handle frag options. */
> +		if (unlikely((fragment_offset == 0) &&
> +			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) &&
> +			    (ipopt_len))) {
> +			__create_ipopt_frag_hdr((uint8_t *)in_hdr,
> +				&ipopt_len, ipopt_frag_hdr);

Can we probably do that before the loop (as we have to do it only once anyway?
Something like:

....
ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
if (ipopt_len > RTE_IPOPT_MAX_LEN)
	return -EINVAL;
if (ipopt_len != 0)
	ipopt_len = __create_ipopt_frag_hdr((in_hdr, ipopt_frag_hdr, ipopt_len);
....

And then:
while (likely(more_in_segs)) {
	...
	if (ipopt_len ! = 0)
		in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;	
}

> +
> +			fragment_offset = (uint16_t)(fragment_offset +
> +				out_pkt->pkt_len - header_len);
> 
> -		out_pkt->l3_len = header_len;
> +			out_pkt->l3_len = header_len;
> +
> +			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
> +			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +		} else {
> +			fragment_offset = (uint16_t)(fragment_offset +
> +			    out_pkt->pkt_len - header_len);
> +
> +			out_pkt->l3_len = header_len;
> +		}
> 
>  		/* Write the fragment to the output list */
>  		pkts_out[out_pkt_pos] = out_pkt;
> --
> 1.8.3.1

Re:RE: [PATCH v3] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]

Hi Konstantin,
> These macros are dups for what we have in rte_ipv4_fragmentation.c
> Would probably make sense to name them RTE_IPV4_IPOPT_... and put them 
> in some public .h to avoid duplication.
I named them RTE_IPV4_IPOPT_xxx and put them in "rte_ip_frag.h".


> Could you clarify what this macro does?
> BTW, I assume it is a local one?
> If so, no need for RTE_ prefix.
Yes,it is a local macro.I will cancel the RTE_ prefix.It is a toggle switch used as a different way to assemble frag test data.It is convenient for users to use different ways to assemble test data.
> Instead of returning void and having out parameter (ipopt_len),
> why just not make it a return value?
> static inline uint16_t
> __create_ipopt_frag_hdr(const uint8_t *iph, uint8_t * ipopt_frag_hdr, uint16_t len)
> {
> 	....
> 	return ipopt_len;
> }
> We probably can update ihl once at the very end of this function.
Ok,I will modify it this way,Thank you for your advice.
> Can we probably do that before the loop (as we have to do it only once anyway?
> Something like:

> ....
> ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
> if (ipopt_len > RTE_IPOPT_MAX_LEN)
> 	return -EINVAL;
> if (ipopt_len != 0)
> 	ipopt_len = __create_ipopt_frag_hdr((in_hdr, ipopt_frag_hdr, ipopt_len);
> ....

> And then:
> while (likely(more_in_segs)) {
> 	...
> 	if (ipopt_len ! = 0)
> 		in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;	
> }
The modified code is as follows：
ipopt_len = header_len - sizeof(struct rte_ipv4_hdr); if (unlikely(ipopt_len > RTE_IPV4_IPOPT_MAX_LEN)) return -EINVAL; else if (ipopt_len == 0) /* Used to mark without processing frag. */ ipopt_len = RTE_IPV4_IPOPT_MAX_LEN + 1; /* The first frag. */ else if ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0) /* Create a separate IP header to handle frag options. */ ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr, ipopt_len, ipopt_frag_hdr);

while (likely(more_in_segs)) {
                ...
		if (unlikely((fragment_offset == 0) &&
			    (ipopt_len <= RTE_IPV4_IPOPT_MAX_LEN) &&
			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
			fragment_offset = (uint16_t)(fragment_offset +
				out_pkt->pkt_len - header_len);
			out_pkt->l3_len = header_len;

			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
		} else {
			fragment_offset = (uint16_t)(fragment_offset +
				out_pkt->pkt_len - header_len);
			out_pkt->l3_len = header_len;
		}



}
These two pieces of code were previously merged together.It doesn't look as brief as before.I would like to hear from you.
Some minor issues：
1. There are some RTE_ prefixes in the rte_ipv4_fragmentation.c.Do I need to move to a public header file?
    /* Fragment Offset */
#define RTE_IPV4_HDR_DF_SHIFT 14 #define RTE_IPV4_HDR_MF_SHIFT 13 #define RTE_IPV4_HDR_FO_SHIFT 3
2. Some comments are in the following format：/**< xxx */,What does this symbol(**<) mean?


Huichao,Cai

[-- Attachment #2: Type: text/html, Size: 9808 bytes --]

[PATCH v4] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 319 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ip_frag.h            |   6 +
 lib/ip_frag/rte_ipv4_fragmentation.c |  70 +++++++-
 3 files changed, 372 insertions(+), 23 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..f6ff2d0 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,109 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+#define IPV4_IPOPT_MANUAL
+
+#ifdef IPV4_IPOPT_MANUAL
+uint8_t expected_first_frag_ipv4_opts_copied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_copied[] = {
+	0x83, 0x07, 0x04, 0xc0,
+	0xa8, 0xe3, 0x96, 0x00,
+};
+
+uint8_t expected_first_frag_ipv4_opts_nocopied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_nocopied[0];
+#else
+/**
+ * IPv4 Options
+ */
+struct test_ipv4_opt {
+	__extension__
+	union {
+		uint8_t type;		    /**< option type */
+		struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+			uint8_t number:5;   /**< option number */
+			uint8_t category:2; /**< option class */
+			uint8_t copied:1;   /**< option copy flag */
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+			uint8_t copied:1;   /**< option copy flag */
+			uint8_t category:2; /**< option class */
+			uint8_t number:5;   /**< option number */
+#endif
+		} s_type;
+	};
+	uint8_t length;			    /**< option length */
+	uint8_t pointer;		    /**< option pointer */
+	uint8_t data[37];		    /**< option data */
+} __rte_packed;
+
+struct test_ipv4_opt test_ipv4_opts_copied[] = {
+	{
+		.s_type.copied = 0,
+		.s_type.category = 0,
+		.s_type.number = 7,
+		.length = 11,
+		.pointer = 4,
+	},
+	{
+		.s_type.copied = 1,
+		.s_type.category = 0,
+		.s_type.number = 3,
+		.length = 7,
+		.pointer = 4,
+		.data[0] = 0xc0,
+		.data[1] = 0xa8,
+		.data[2] = 0xe3,
+		.data[3] = 0x96,
+	},
+};
+
+struct test_ipv4_opt test_ipv4_opts_nocopied[] = {
+	{
+		.s_type.copied = 0,
+		.s_type.category = 0,
+		.s_type.number = 7,
+		.length = 11,
+		.pointer = 4,
+	},
+};
+#endif
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	bool opt_copied;		 /**< ip option copied flag */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPV4_IPOPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +187,108 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
+	struct test_opt_data *expected_opt)
+{
+#ifdef IPV4_IPOPT_MANUAL
+	if (is_first_frag) {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_copied,
+				sizeof(expected_first_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_nocopied,
+				sizeof(expected_first_frag_ipv4_opts_nocopied));
+		}
+	} else {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_copied,
+				sizeof(expected_sub_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_nocopied,
+				sizeof(expected_sub_frag_ipv4_opts_nocopied));
+		}
+	}
+#else
+	uint16_t i;
+	uint16_t pos = 0;
+	expected_opt->len = 0;
+	struct test_ipv4_opt *opts = NULL;
+	uint16_t opt_num;
+
+	if (opt_copied) {
+		opts = test_ipv4_opts_copied;
+		opt_num = RTE_DIM(test_ipv4_opts_copied);
+	} else {
+		opts = test_ipv4_opts_nocopied;
+		opt_num = RTE_DIM(test_ipv4_opts_nocopied);
+	}
+
+	for (i = 0; i < opt_num; i++) {
+		if (unlikely(pos + opts[i].length >
+				RTE_IPV4_IPOPT_MAX_LEN))
+			return;
+
+		if (is_first_frag) {
+			rte_memcpy(expected_opt->data + pos, &opts[i],
+				opts[i].length);
+			expected_opt->len += opts[i].length;
+			pos += opts[i].length;
+		} else {
+			if (opts[i].s_type.copied) {
+				rte_memcpy(expected_opt->data + pos,
+					&opts[i],
+					opts[i].length);
+				expected_opt->len += opts[i].length;
+				pos += opts[i].length;
+			}
+		}
+	}
+
+	expected_opt->len = RTE_ALIGN_CEIL(expected_opt->len, 4);
+	memset(expected_opt->data + pos, RTE_IPV4_IPOPT_EOL,
+		expected_opt->len - pos);
+#endif
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag, bool opt_copied)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, opt_copied, &opt);
+
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +315,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +373,45 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv, bool opt_copied)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			opt->opt_copied = opt_copied;
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (likely(opt_len <= RTE_IPV4_IPOPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPV4_IPOPT_MAX_LEN;
+				memset(opt->data, RTE_IPV4_IPOPT_EOL,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +431,52 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
+		bool opt_copied;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, false},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2046, 0x008C}, true, true, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, true, true, true},
+		 /* The middle fragment */
 		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
-
+		  {0x200D, 0x2012, 0x2017}, true, false, true},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x201F, 0x0024}, true, false, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           4,
+		  {0x2000, 0x2004, 0x2008, 0x200C}, true, true, false},
+		 /* The middle fragment */
+		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x200D, 0x2013, 0x2019}, true, false, false},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x2020, 0x0026}, true, false, false},
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, false},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +495,10 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag,
+					      tests[i].opt_copied);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +523,20 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len, opt_res,
+					tests[i].ipv, tests[i].opt_copied);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +545,36 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPV4_IPOPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPV4_IPOPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						opt_res[j].opt_copied,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (len:%u)%s with (len:%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ip_frag.h b/lib/ip_frag/rte_ip_frag.h
index 7d2abe1..f337d1e 100644
--- a/lib/ip_frag/rte_ip_frag.h
+++ b/lib/ip_frag/rte_ip_frag.h
@@ -27,6 +27,12 @@
 
 struct rte_mbuf;
 
+/* IP options */
+#define RTE_IPV4_IPOPT_EOL       0
+#define RTE_IPV4_IPOPT_NOP       1
+#define RTE_IPV4_IPOPT_COPIED(v) ((v) & 0x80)
+#define RTE_IPV4_IPOPT_MAX_LEN   40
+
 /** death row size (in packets) */
 #define RTE_IP_FRAG_DEATH_ROW_LEN 32
 
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..1e655a7 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -22,6 +22,8 @@
 
 #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
 
+#define IPV4_HDR_MAX_LEN			60
+
 static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
 		const struct rte_ipv4_hdr *src, uint16_t header_len,
 		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
@@ -41,6 +43,49 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
+	uint16_t ipopt_len, uint8_t *ipopt_frag_hdr)
+{
+	uint16_t len = ipopt_len;
+	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+
+	ipopt_len = 0;
+	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
+
+	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
+
+	while (len > 0) {
+		if (unlikely(*p_opt == RTE_IPV4_IPOPT_NOP)) {
+			len--;
+			p_opt++;
+			continue;
+		} else if (unlikely(*p_opt == RTE_IPV4_IPOPT_EOL))
+			break;
+
+		if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
+			break;
+
+		if (RTE_IPV4_IPOPT_COPIED(*p_opt)) {
+			rte_memcpy(ipopt_frag_hdr + ipopt_len,
+				p_opt, p_opt[1]);
+			ipopt_len += p_opt[1];
+		}
+
+		len -= p_opt[1];
+		p_opt += p_opt[1];
+	}
+
+	len = RTE_ALIGN_CEIL(ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
+	memset(ipopt_frag_hdr + ipopt_len,
+		RTE_IPV4_IPOPT_EOL, len - ipopt_len);
+	ipopt_len = len;
+	iph_opt->ihl = (sizeof(struct rte_ipv4_hdr) + ipopt_len) /
+		RTE_IPV4_IHL_MULTIPLIER;
+
+	return ipopt_len;
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -76,6 +121,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	uint32_t more_in_segs;
 	uint16_t fragment_offset, flag_offset, frag_size, header_len;
 	uint16_t frag_bytes_remaining;
+	uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
+	uint16_t ipopt_len;
 
 	/*
 	 * Formal parameter checking.
@@ -118,6 +165,10 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	out_pkt_pos = 0;
 	fragment_offset = 0;
 
+	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
+	if (unlikely(ipopt_len > RTE_IPV4_IPOPT_MAX_LEN))
+		return -EINVAL;
+
 	more_in_segs = 1;
 	while (likely(more_in_segs)) {
 		struct rte_mbuf *out_pkt = NULL, *out_seg_prev = NULL;
@@ -188,10 +239,21 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
-		fragment_offset = (uint16_t)(fragment_offset +
-		    out_pkt->pkt_len - header_len);
-
-		out_pkt->l3_len = header_len;
+		if (unlikely((fragment_offset == 0) && (ipopt_len) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
+			ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr,
+				ipopt_len, ipopt_frag_hdr);
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+
+			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
+			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+		} else {
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+		}
 
 		/* Write the fragment to the output list */
 		pkts_out[out_pkt_pos] = out_pkt;
-- 
1.8.3.1

RE: [PATCH v4] ip_frag: add IPv4 options fragment and test data

[Ananyev, Konstantin]

Hi Huichao,

> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---
>  app/test/test_ipfrag.c               | 319 ++++++++++++++++++++++++++++++++---
>  lib/ip_frag/rte_ip_frag.h            |   6 +
>  lib/ip_frag/rte_ipv4_fragmentation.c |  70 +++++++-
>  3 files changed, 372 insertions(+), 23 deletions(-)
> 
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> index 1ced25a..f6ff2d0 100644
> --- a/app/test/test_ipfrag.c
> +++ b/app/test/test_ipfrag.c
> @@ -18,10 +18,109 @@
>  #define NUM_MBUFS 128
>  #define BURST 32
> 
> +#define IPV4_IPOPT_MANUAL
> +
> +#ifdef IPV4_IPOPT_MANUAL

Could you explain why do we need that define at all?
As I can read the code, right now IPV4_IPOPT_MANUAL is always defined,
so all '#else' blocks are simply dead code.
Is there any reason to keep it?
If so, then the code probably need to be re-ordered somehow, 
to make '#else' part to be enabled and executed: 
let say a separate test-case(s), and/or separate function or extra parameter
for  test_get_ipv4_opt().
Konstantin

Re:RE: [PATCH v4] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]







Hi Konstantin,

>Could you explain why do we need that define at all?
>As I can read the code, right now IPV4_IPOPT_MANUAL is always defined,
>so all '#else' blocks are simply dead code.
>Is there any reason to keep it?
>If so, then the code probably need to be re-ordered somehow, 
>to make '#else' part to be enabled and executed: 
>let say a separate test-case(s), and/or separate function or extra parameter

>for  test_get_ipv4_opt().


The code for the '#else' part is a relatively simple way to organize the test data, there is no need to stitch all the data together manually, but it is not as freely organized as the '#if' part of the code, and it was intended that as an alternative, I will remove the code of the '#else' part and resend the patch.


Huichao Cai

[-- Attachment #2: Type: text/html, Size: 1154 bytes --]

[PATCH v5] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 219 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ip_frag.h            |   6 +
 lib/ip_frag/rte_ipv4_fragmentation.c |  70 ++++++++++-
 3 files changed, 272 insertions(+), 23 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..8289a60 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,50 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+uint8_t expected_first_frag_ipv4_opts_copied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_copied[] = {
+	0x83, 0x07, 0x04, 0xc0,
+	0xa8, 0xe3, 0x96, 0x00,
+};
+
+uint8_t expected_first_frag_ipv4_opts_nocopied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_nocopied[0];
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	bool opt_copied;		 /**< ip option copied flag */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPV4_IPOPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +128,67 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
+	struct test_opt_data *expected_opt)
+{
+	if (is_first_frag) {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_copied,
+				sizeof(expected_first_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_nocopied,
+				sizeof(expected_first_frag_ipv4_opts_nocopied));
+		}
+	} else {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_copied,
+				sizeof(expected_sub_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_nocopied,
+				sizeof(expected_sub_frag_ipv4_opts_nocopied));
+		}
+	}
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag, bool opt_copied)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
+
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, opt_copied, &opt);
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +215,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +273,45 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv, bool opt_copied)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			opt->opt_copied = opt_copied;
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (likely(opt_len <= RTE_IPV4_IPOPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPV4_IPOPT_MAX_LEN;
+				memset(opt->data, RTE_IPV4_IPOPT_EOL,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +331,52 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
+		bool opt_copied;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, false},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2046, 0x008C}, true, true, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, true, true, true},
+		 /* The middle fragment */
 		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
-
+		  {0x200D, 0x2012, 0x2017}, true, false, true},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x201F, 0x0024}, true, false, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           4,
+		  {0x2000, 0x2004, 0x2008, 0x200C}, true, true, false},
+		 /* The middle fragment */
+		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x200D, 0x2013, 0x2019}, true, false, false},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x2020, 0x0026}, true, false, false},
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, false},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +395,10 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag,
+					      tests[i].opt_copied);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +423,20 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len, opt_res,
+					tests[i].ipv, tests[i].opt_copied);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +445,36 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPV4_IPOPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPV4_IPOPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						opt_res[j].opt_copied,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (len:%u)%s with (len:%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ip_frag.h b/lib/ip_frag/rte_ip_frag.h
index 7d2abe1..f337d1e 100644
--- a/lib/ip_frag/rte_ip_frag.h
+++ b/lib/ip_frag/rte_ip_frag.h
@@ -27,6 +27,12 @@
 
 struct rte_mbuf;
 
+/* IP options */
+#define RTE_IPV4_IPOPT_EOL       0
+#define RTE_IPV4_IPOPT_NOP       1
+#define RTE_IPV4_IPOPT_COPIED(v) ((v) & 0x80)
+#define RTE_IPV4_IPOPT_MAX_LEN   40
+
 /** death row size (in packets) */
 #define RTE_IP_FRAG_DEATH_ROW_LEN 32
 
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..1e655a7 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -22,6 +22,8 @@
 
 #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
 
+#define IPV4_HDR_MAX_LEN			60
+
 static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
 		const struct rte_ipv4_hdr *src, uint16_t header_len,
 		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
@@ -41,6 +43,49 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
+	uint16_t ipopt_len, uint8_t *ipopt_frag_hdr)
+{
+	uint16_t len = ipopt_len;
+	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+
+	ipopt_len = 0;
+	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
+
+	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
+
+	while (len > 0) {
+		if (unlikely(*p_opt == RTE_IPV4_IPOPT_NOP)) {
+			len--;
+			p_opt++;
+			continue;
+		} else if (unlikely(*p_opt == RTE_IPV4_IPOPT_EOL))
+			break;
+
+		if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
+			break;
+
+		if (RTE_IPV4_IPOPT_COPIED(*p_opt)) {
+			rte_memcpy(ipopt_frag_hdr + ipopt_len,
+				p_opt, p_opt[1]);
+			ipopt_len += p_opt[1];
+		}
+
+		len -= p_opt[1];
+		p_opt += p_opt[1];
+	}
+
+	len = RTE_ALIGN_CEIL(ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
+	memset(ipopt_frag_hdr + ipopt_len,
+		RTE_IPV4_IPOPT_EOL, len - ipopt_len);
+	ipopt_len = len;
+	iph_opt->ihl = (sizeof(struct rte_ipv4_hdr) + ipopt_len) /
+		RTE_IPV4_IHL_MULTIPLIER;
+
+	return ipopt_len;
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -76,6 +121,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	uint32_t more_in_segs;
 	uint16_t fragment_offset, flag_offset, frag_size, header_len;
 	uint16_t frag_bytes_remaining;
+	uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
+	uint16_t ipopt_len;
 
 	/*
 	 * Formal parameter checking.
@@ -118,6 +165,10 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	out_pkt_pos = 0;
 	fragment_offset = 0;
 
+	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
+	if (unlikely(ipopt_len > RTE_IPV4_IPOPT_MAX_LEN))
+		return -EINVAL;
+
 	more_in_segs = 1;
 	while (likely(more_in_segs)) {
 		struct rte_mbuf *out_pkt = NULL, *out_seg_prev = NULL;
@@ -188,10 +239,21 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
-		fragment_offset = (uint16_t)(fragment_offset +
-		    out_pkt->pkt_len - header_len);
-
-		out_pkt->l3_len = header_len;
+		if (unlikely((fragment_offset == 0) && (ipopt_len) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
+			ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr,
+				ipopt_len, ipopt_frag_hdr);
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+
+			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
+			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+		} else {
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+		}
 
 		/* Write the fragment to the output list */
 		pkts_out[out_pkt_pos] = out_pkt;
-- 
1.8.3.1

RE: [PATCH v5] ip_frag: add IPv4 options fragment and test data

[Ananyev, Konstantin]

> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 1.8.3.1

Re:RE: [PATCH v5] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 219 bytes --]

Hi Konstantin,


This patch has a test case failure:ci/iol-broadcom-Functional.
Failed Tests:
		- mtu_update
		- scatter
The same goes for many other patches,Do I need to deal with it, how to deal with it?


Huichao,Cai

[-- Attachment #2: Type: text/html, Size: 1222 bytes --]

RE: Re:RE: [PATCH v5] ip_frag: add IPv4 options fragment and test data

[Ananyev, Konstantin]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

Hi Huichao,

In general yes, it is developer responsibility to address any issues with his/her patches.
In that particular case, looking at the logs, it seems to be some misconfiguration
on test-machine not related anyhow to your changes.
BTW, there are few similar failures with other patches at about the same date:
https://lab.dpdk.org/results/dashboard/patchsets/21562/
https://lab.dpdk.org/results/dashboard/patchsets/21546/
Which again, makes me think that  it is just a tesc-config related failure.
What is the best way to deal with it?
Probably the easiest and safest thing - to resubmit the patch to force
another run of test harness.
Aaron, is there any better way to deal with it?
Thanks
Konstantin


From: Huichao Cai <chcchc88@163.com>
Sent: Wednesday, April 6, 2022 2:22 AM
To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
Cc: dev@dpdk.org
Subject: Re:RE: [PATCH v5] ip_frag: add IPv4 options fragment and test data

Hi Konstantin,

This patch has a test case failure:ci/iol-broadcom-Functional.

Failed Tests:

               - mtu_update

               - scatter
The same goes for many other patches,Do I need to deal with it, how to deal with it?

Huichao,Cai




[-- Attachment #2: Type: text/html, Size: 10660 bytes --]

Re: [PATCH v5] ip_frag: add IPv4 options fragment and test data

[Aaron Conole]

Hi,

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

> Hi Huichao,
>
>  
>
> In general yes, it is developer responsibility to address any issues with his/her patches.

+1

> In that particular case, looking at the logs, it seems to be some misconfiguration
>
> on test-machine not related anyhow to your changes.
>
> BTW, there are few similar failures with other patches at about the same date:
>
> https://lab.dpdk.org/results/dashboard/patchsets/21562/
>
> https://lab.dpdk.org/results/dashboard/patchsets/21546/
>
> Which again, makes me think that  it is just a tesc-config related failure.
>
> What is the best way to deal with it?

Agreed.  I've CC'd UNH lab, but in this case I think these are the BRCM
managed systems.

> Probably the easiest and safest thing – to resubmit the patch to force
>
> another run of test harness.
>
> Aaron, is there any better way to deal with it?

At the moment, no.  We do have an effort for resubmits to be requested -
but that hasn't been completed yet.

> Thanks
>
> Konstantin
>
>  
>
>  
>
> From: Huichao Cai <chcchc88@163.com> 
> Sent: Wednesday, April 6, 2022 2:22 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re:RE: [PATCH v5] ip_frag: add IPv4 options fragment and test data
>
>  
>
> Hi Konstantin,
>
>  
>
> This patch has a test case failure:ci/iol-broadcom-Functional.
>
> Failed Tests:
>
>                - mtu_update
>
>                - scatter
>
> The same goes for many other patches,Do I need to deal with it, how to deal with it?
>
>  
>
> Huichao,Cai

Re:Re: [PATCH v5] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 162 bytes --]

Hi everyone,


I have resubmitted the patch.Thanks.
https://patchwork.dpdk.org/project/dpdk/patch/1649649325-1942-1-git-send-email-chcchc88@163.com/


Huichao,Cai

[-- Attachment #2: Type: text/html, Size: 585 bytes --]

[PATCH v6] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 219 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ip_frag.h            |   6 +
 lib/ip_frag/rte_ipv4_fragmentation.c |  70 ++++++++++-
 3 files changed, 272 insertions(+), 23 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..8289a60 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,50 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+uint8_t expected_first_frag_ipv4_opts_copied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_copied[] = {
+	0x83, 0x07, 0x04, 0xc0,
+	0xa8, 0xe3, 0x96, 0x00,
+};
+
+uint8_t expected_first_frag_ipv4_opts_nocopied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_nocopied[0];
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	bool opt_copied;		 /**< ip option copied flag */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPV4_IPOPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +128,67 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
+	struct test_opt_data *expected_opt)
+{
+	if (is_first_frag) {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_copied,
+				sizeof(expected_first_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_nocopied,
+				sizeof(expected_first_frag_ipv4_opts_nocopied));
+		}
+	} else {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_copied,
+				sizeof(expected_sub_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_nocopied,
+				sizeof(expected_sub_frag_ipv4_opts_nocopied));
+		}
+	}
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag, bool opt_copied)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
+
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, opt_copied, &opt);
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +215,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +273,45 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv, bool opt_copied)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			opt->opt_copied = opt_copied;
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (likely(opt_len <= RTE_IPV4_IPOPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPV4_IPOPT_MAX_LEN;
+				memset(opt->data, RTE_IPV4_IPOPT_EOL,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +331,52 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
+		bool opt_copied;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, false},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2046, 0x008C}, true, true, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, true, true, true},
+		 /* The middle fragment */
 		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
-
+		  {0x200D, 0x2012, 0x2017}, true, false, true},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x201F, 0x0024}, true, false, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           4,
+		  {0x2000, 0x2004, 0x2008, 0x200C}, true, true, false},
+		 /* The middle fragment */
+		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x200D, 0x2013, 0x2019}, true, false, false},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x2020, 0x0026}, true, false, false},
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, false},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +395,10 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag,
+					      tests[i].opt_copied);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +423,20 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len, opt_res,
+					tests[i].ipv, tests[i].opt_copied);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +445,36 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPV4_IPOPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPV4_IPOPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						opt_res[j].opt_copied,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (len:%u)%s with (len:%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ip_frag.h b/lib/ip_frag/rte_ip_frag.h
index 7d2abe1..f337d1e 100644
--- a/lib/ip_frag/rte_ip_frag.h
+++ b/lib/ip_frag/rte_ip_frag.h
@@ -27,6 +27,12 @@
 
 struct rte_mbuf;
 
+/* IP options */
+#define RTE_IPV4_IPOPT_EOL       0
+#define RTE_IPV4_IPOPT_NOP       1
+#define RTE_IPV4_IPOPT_COPIED(v) ((v) & 0x80)
+#define RTE_IPV4_IPOPT_MAX_LEN   40
+
 /** death row size (in packets) */
 #define RTE_IP_FRAG_DEATH_ROW_LEN 32
 
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..1e655a7 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -22,6 +22,8 @@
 
 #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
 
+#define IPV4_HDR_MAX_LEN			60
+
 static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
 		const struct rte_ipv4_hdr *src, uint16_t header_len,
 		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
@@ -41,6 +43,49 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
+	uint16_t ipopt_len, uint8_t *ipopt_frag_hdr)
+{
+	uint16_t len = ipopt_len;
+	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+
+	ipopt_len = 0;
+	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
+
+	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
+
+	while (len > 0) {
+		if (unlikely(*p_opt == RTE_IPV4_IPOPT_NOP)) {
+			len--;
+			p_opt++;
+			continue;
+		} else if (unlikely(*p_opt == RTE_IPV4_IPOPT_EOL))
+			break;
+
+		if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
+			break;
+
+		if (RTE_IPV4_IPOPT_COPIED(*p_opt)) {
+			rte_memcpy(ipopt_frag_hdr + ipopt_len,
+				p_opt, p_opt[1]);
+			ipopt_len += p_opt[1];
+		}
+
+		len -= p_opt[1];
+		p_opt += p_opt[1];
+	}
+
+	len = RTE_ALIGN_CEIL(ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
+	memset(ipopt_frag_hdr + ipopt_len,
+		RTE_IPV4_IPOPT_EOL, len - ipopt_len);
+	ipopt_len = len;
+	iph_opt->ihl = (sizeof(struct rte_ipv4_hdr) + ipopt_len) /
+		RTE_IPV4_IHL_MULTIPLIER;
+
+	return ipopt_len;
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -76,6 +121,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	uint32_t more_in_segs;
 	uint16_t fragment_offset, flag_offset, frag_size, header_len;
 	uint16_t frag_bytes_remaining;
+	uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
+	uint16_t ipopt_len;
 
 	/*
 	 * Formal parameter checking.
@@ -118,6 +165,10 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	out_pkt_pos = 0;
 	fragment_offset = 0;
 
+	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
+	if (unlikely(ipopt_len > RTE_IPV4_IPOPT_MAX_LEN))
+		return -EINVAL;
+
 	more_in_segs = 1;
 	while (likely(more_in_segs)) {
 		struct rte_mbuf *out_pkt = NULL, *out_seg_prev = NULL;
@@ -188,10 +239,21 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
-		fragment_offset = (uint16_t)(fragment_offset +
-		    out_pkt->pkt_len - header_len);
-
-		out_pkt->l3_len = header_len;
+		if (unlikely((fragment_offset == 0) && (ipopt_len) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
+			ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr,
+				ipopt_len, ipopt_frag_hdr);
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+
+			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
+			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+		} else {
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+		}
 
 		/* Write the fragment to the output list */
 		pkts_out[out_pkt_pos] = out_pkt;
-- 
1.8.3.1

Re: [PATCH v6] ip_frag: add IPv4 options fragment and test data

[Thomas Monjalon]

11/04/2022 05:55, Huichao Cai:
> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>

You forgot the ack given on v5:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks.

Re: [PATCH v6] ip_frag: add IPv4 options fragment and test data

[Thomas Monjalon]

14/04/2022 15:14, Thomas Monjalon:
> 11/04/2022 05:55, Huichao Cai:
> > According to RFC791,the options may appear or not in datagrams.
> > They must be implemented by all IP modules (host and gateways).
> > What is optional is their transmission in any particular datagram,
> > not their implementation.So we have to deal with it during the
> > fragmenting process.Add some test data for the IPv4 header optional
> > field fragmenting.
> > 
> > Signed-off-by: Huichao Cai <chcchc88@163.com>
> 
> You forgot the ack given on v5:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> Applied, thanks.

No sorry, it cannot be applied as-is.

Some IP constants should be defined in lib/net/rte_ip.h
and should be named RTE_IPV4_HDR_OPT_* for consistency.

Re:Re: [PATCH v6] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 208 bytes --]




>No sorry, it cannot be applied as-is.
>
>Some IP constants should be defined in lib/net/rte_ip.h
>and should be named RTE_IPV4_HDR_OPT_* for consistency.
Ok,I will modify it and resubmit the patch.Thanks.

[-- Attachment #2: Type: text/html, Size: 396 bytes --]

[PATCH v7] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

According to RFC791,the options may appear or not in datagrams.
They must be implemented by all IP modules (host and gateways).
What is optional is their transmission in any particular datagram,
not their implementation.So we have to deal with it during the
fragmenting process.Add some test data for the IPv4 header optional
field fragmenting.

Signed-off-by: Huichao Cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c               | 219 ++++++++++++++++++++++++++++++++---
 lib/ip_frag/rte_ipv4_fragmentation.c |  70 ++++++++++-
 lib/net/rte_ip.h                     |   6 +
 3 files changed, 272 insertions(+), 23 deletions(-)

diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
index 1ced25a..610a86b 100644
--- a/app/test/test_ipfrag.c
+++ b/app/test/test_ipfrag.c
@@ -18,10 +18,50 @@
 #define NUM_MBUFS 128
 #define BURST 32
 
+uint8_t expected_first_frag_ipv4_opts_copied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x83,
+	0x07, 0x04, 0xc0, 0xa8,
+	0xe3, 0x96, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_copied[] = {
+	0x83, 0x07, 0x04, 0xc0,
+	0xa8, 0xe3, 0x96, 0x00,
+};
+
+uint8_t expected_first_frag_ipv4_opts_nocopied[] = {
+	0x07, 0x0b, 0x04, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+};
+
+uint8_t expected_sub_frag_ipv4_opts_nocopied[0];
+
+struct test_opt_data {
+	bool is_first_frag;		 /**< offset is 0 */
+	bool opt_copied;		 /**< ip option copied flag */
+	uint16_t len;			 /**< option data len */
+	uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
+};
+
 static struct rte_mempool *pkt_pool,
 			  *direct_pool,
 			  *indirect_pool;
 
+static inline void
+hex_to_str(uint8_t *hex, uint16_t len, char *str)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		sprintf(str, "%02x", hex[i]);
+		str += 2;
+	}
+	*str = 0;
+}
+
 static int
 setup_buf_pool(void)
 {
@@ -88,23 +128,67 @@ static void ut_teardown(void)
 {
 }
 
+static inline void
+test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
+	struct test_opt_data *expected_opt)
+{
+	if (is_first_frag) {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_copied,
+				sizeof(expected_first_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_first_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_first_frag_ipv4_opts_nocopied,
+				sizeof(expected_first_frag_ipv4_opts_nocopied));
+		}
+	} else {
+		if (opt_copied) {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_copied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_copied,
+				sizeof(expected_sub_frag_ipv4_opts_copied));
+		} else {
+			expected_opt->len =
+				sizeof(expected_sub_frag_ipv4_opts_nocopied);
+			rte_memcpy(expected_opt->data,
+				expected_sub_frag_ipv4_opts_nocopied,
+				sizeof(expected_sub_frag_ipv4_opts_nocopied));
+		}
+	}
+}
+
 static void
-v4_allocate_packet_of(struct rte_mbuf *b, int fill,
-		      size_t s, int df, uint8_t mf, uint16_t off,
-		      uint8_t ttl, uint8_t proto, uint16_t pktid)
+v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
+	int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
+	uint16_t pktid, bool have_opt, bool is_first_frag, bool opt_copied)
 {
 	/* Create a packet, 2k bytes long */
 	b->data_off = 0;
 	char *data = rte_pktmbuf_mtod(b, char *);
-	rte_be16_t fragment_offset = 0;	/**< fragmentation offset */
+	rte_be16_t fragment_offset = 0;	/* fragmentation offset */
+	uint16_t iph_len;
+	struct test_opt_data opt;
+
+	opt.len = 0;
+
+	if (have_opt)
+		test_get_ipv4_opt(is_first_frag, opt_copied, &opt);
 
-	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
+	iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
+	memset(data, fill, iph_len + s);
 
 	struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
 
-	hdr->version_ihl = 0x45; /* standard IP header... */
+	hdr->version_ihl = 0x40; /* ipv4 */
+	hdr->version_ihl += (iph_len / 4);
 	hdr->type_of_service = 0;
-	b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
+	b->pkt_len = s + iph_len;
 	b->data_len = b->pkt_len;
 	hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
 	hdr->packet_id = rte_cpu_to_be_16(pktid);
@@ -131,6 +215,8 @@ static void ut_teardown(void)
 	hdr->hdr_checksum = 0;
 	hdr->src_addr = rte_cpu_to_be_32(0x8080808);
 	hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
+
+	rte_memcpy(hdr + 1, opt.data, opt.len);
 }
 
 static void
@@ -187,6 +273,45 @@ static void ut_teardown(void)
 	}
 }
 
+static inline void
+test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
+	struct test_opt_data *opt, int ipv, bool opt_copied)
+{
+	int32_t i;
+
+	for (i = 0; i < num; i++) {
+		if (ipv == 4) {
+			struct rte_ipv4_hdr *iph =
+			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
+			uint16_t header_len = (iph->version_ihl &
+				RTE_IPV4_HDR_IHL_MASK) *
+				RTE_IPV4_IHL_MULTIPLIER;
+			uint16_t opt_len = header_len -
+				sizeof(struct rte_ipv4_hdr);
+
+			opt->opt_copied = opt_copied;
+
+			if ((rte_be_to_cpu_16(iph->fragment_offset) &
+				    RTE_IPV4_HDR_OFFSET_MASK) == 0)
+				opt->is_first_frag = true;
+			else
+				opt->is_first_frag = false;
+
+			if (likely(opt_len <= RTE_IPV4_HDR_OPT_MAX_LEN)) {
+				char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
+				    char *, sizeof(struct rte_ipv4_hdr));
+				opt->len = opt_len;
+				rte_memcpy(opt->data, iph_opt, opt_len);
+			} else {
+				opt->len = RTE_IPV4_HDR_OPT_MAX_LEN;
+				memset(opt->data, RTE_IPV4_HDR_OPT_EOL,
+				    sizeof(opt->data));
+			}
+			opt++;
+		}
+	}
+}
+
 static int
 test_ip_frag(void)
 {
@@ -206,32 +331,52 @@ static void ut_teardown(void)
 		uint16_t pkt_id;
 		int      expected_frags;
 		uint16_t expected_fragment_offset[BURST];
+		bool have_opt;
+		bool is_first_frag;
+		bool opt_copied;
 	} tests[] = {
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
-		  {0x2000, 0x009D}},
+		  {0x2000, 0x009D}, false},
 		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2048, 0x0090}, false},
 		 {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
 		 {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
-		  {0x2000, 0x2048, 0x0090}},
+		  {0x2000, 0x2046, 0x008C}, true, true, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
+		  {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, true, true, true},
+		 /* The middle fragment */
 		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
-		  {0x200D, 0x2013, 0x2019}},
-
+		  {0x200D, 0x2012, 0x2017}, true, false, true},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x201F, 0x0024}, true, false, true},
+		 /* The first fragment */
+		 {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           4,
+		  {0x2000, 0x2004, 0x2008, 0x200C}, true, true, false},
+		 /* The middle fragment */
+		 {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x200D, 0x2013, 0x2019}, true, false, false},
+		 /* The last fragment */
+		 {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
+		  {0x201A, 0x2020, 0x0026}, true, false, false},
 		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04D0}},
+		  {0x0001, 0x04D0}, false},
 		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 		 {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
 		 {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
-		  {0x0001, 0x04E0}},
+		  {0x0001, 0x04E0}, false},
 	};
 
 	for (i = 0; i < RTE_DIM(tests); i++) {
 		int32_t len = 0;
 		uint16_t fragment_offset[BURST];
+		struct test_opt_data opt_res[BURST];
+		struct test_opt_data opt_exp;
 		uint16_t pktid = tests[i].pkt_id;
 		struct rte_mbuf *pkts_out[BURST];
 		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
@@ -250,7 +395,10 @@ static void ut_teardown(void)
 					      tests[i].set_of,
 					      tests[i].ttl,
 					      tests[i].proto,
-					      pktid);
+					      pktid,
+					      tests[i].have_opt,
+					      tests[i].is_first_frag,
+					      tests[i].opt_copied);
 		} else if (tests[i].ipv == 6) {
 			v6_allocate_packet_of(b, 0x41414141,
 					      tests[i].pkt_size,
@@ -275,17 +423,20 @@ static void ut_teardown(void)
 		if (len > 0) {
 			test_get_offset(pkts_out, len,
 			    fragment_offset, tests[i].ipv);
+			if (tests[i].have_opt)
+				test_get_frag_opt(pkts_out, len, opt_res,
+					tests[i].ipv, tests[i].opt_copied);
 			test_free_fragments(pkts_out, len);
 		}
 
-		printf("%zd: checking %d with %d\n", i, len,
+		printf("[check frag number]%zd: checking %d with %d\n", i, len,
 		       tests[i].expected_frags);
 		RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
 				      "Failed case %zd.\n", i);
 
 		if (len > 0) {
 			for (j = 0; j < (size_t)len; j++) {
-				printf("%zd-%zd: checking %d with %d\n",
+				printf("[check offset]%zd-%zd: checking %d with %d\n",
 				    i, j, fragment_offset[j],
 				    rte_cpu_to_be_16(
 					tests[i].expected_fragment_offset[j]));
@@ -294,6 +445,36 @@ static void ut_teardown(void)
 					tests[i].expected_fragment_offset[j]),
 				    "Failed case %zd.\n", i);
 			}
+
+			if (tests[i].have_opt && (tests[i].ipv == 4)) {
+				for (j = 0; j < (size_t)len; j++) {
+					char opt_res_str[2 *
+						RTE_IPV4_HDR_OPT_MAX_LEN + 1];
+					char opt_exp_str[2 *
+						RTE_IPV4_HDR_OPT_MAX_LEN + 1];
+
+					test_get_ipv4_opt(
+						opt_res[j].is_first_frag,
+						opt_res[j].opt_copied,
+						&opt_exp);
+					hex_to_str(opt_res[j].data,
+						opt_res[j].len,
+						opt_res_str);
+					hex_to_str(opt_exp.data,
+						opt_exp.len,
+						opt_exp_str);
+
+					printf(
+						"[check ipv4 option]%zd-%zd: checking (len:%u)%s with (len:%u)%s\n",
+						i, j,
+						opt_res[j].len, opt_res_str,
+						opt_exp.len, opt_exp_str);
+						RTE_TEST_ASSERT_SUCCESS(
+							strcmp(opt_res_str,
+								opt_exp_str),
+						"Failed case %zd.\n", i);
+				}
+			}
 		}
 
 	}
diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
index 2e7739d..a562424 100644
--- a/lib/ip_frag/rte_ipv4_fragmentation.c
+++ b/lib/ip_frag/rte_ipv4_fragmentation.c
@@ -22,6 +22,8 @@
 
 #define	IPV4_HDR_FO_ALIGN			(1 << RTE_IPV4_HDR_FO_SHIFT)
 
+#define IPV4_HDR_MAX_LEN			60
+
 static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
 		const struct rte_ipv4_hdr *src, uint16_t header_len,
 		uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
@@ -41,6 +43,49 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		rte_pktmbuf_free(mb[i]);
 }
 
+static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
+	uint16_t ipopt_len, uint8_t *ipopt_frag_hdr)
+{
+	uint16_t len = ipopt_len;
+	struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+
+	ipopt_len = 0;
+	rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
+	ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
+
+	uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
+
+	while (len > 0) {
+		if (unlikely(*p_opt == RTE_IPV4_HDR_OPT_NOP)) {
+			len--;
+			p_opt++;
+			continue;
+		} else if (unlikely(*p_opt == RTE_IPV4_HDR_OPT_EOL))
+			break;
+
+		if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
+			break;
+
+		if (RTE_IPV4_HDR_OPT_COPIED(*p_opt)) {
+			rte_memcpy(ipopt_frag_hdr + ipopt_len,
+				p_opt, p_opt[1]);
+			ipopt_len += p_opt[1];
+		}
+
+		len -= p_opt[1];
+		p_opt += p_opt[1];
+	}
+
+	len = RTE_ALIGN_CEIL(ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
+	memset(ipopt_frag_hdr + ipopt_len,
+		RTE_IPV4_HDR_OPT_EOL, len - ipopt_len);
+	ipopt_len = len;
+	iph_opt->ihl = (sizeof(struct rte_ipv4_hdr) + ipopt_len) /
+		RTE_IPV4_IHL_MULTIPLIER;
+
+	return ipopt_len;
+}
+
 /**
  * IPv4 fragmentation.
  *
@@ -76,6 +121,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	uint32_t more_in_segs;
 	uint16_t fragment_offset, flag_offset, frag_size, header_len;
 	uint16_t frag_bytes_remaining;
+	uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
+	uint16_t ipopt_len;
 
 	/*
 	 * Formal parameter checking.
@@ -118,6 +165,10 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 	out_pkt_pos = 0;
 	fragment_offset = 0;
 
+	ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
+	if (unlikely(ipopt_len > RTE_IPV4_HDR_OPT_MAX_LEN))
+		return -EINVAL;
+
 	more_in_segs = 1;
 	while (likely(more_in_segs)) {
 		struct rte_mbuf *out_pkt = NULL, *out_seg_prev = NULL;
@@ -188,10 +239,21 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
 		    (uint16_t)out_pkt->pkt_len,
 		    flag_offset, fragment_offset, more_in_segs);
 
-		fragment_offset = (uint16_t)(fragment_offset +
-		    out_pkt->pkt_len - header_len);
-
-		out_pkt->l3_len = header_len;
+		if (unlikely((fragment_offset == 0) && (ipopt_len) &&
+			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
+			ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr,
+				ipopt_len, ipopt_frag_hdr);
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+
+			header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
+			in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
+		} else {
+			fragment_offset = (uint16_t)(fragment_offset +
+				out_pkt->pkt_len - header_len);
+			out_pkt->l3_len = header_len;
+		}
 
 		/* Write the fragment to the output list */
 		pkts_out[out_pkt_pos] = out_pkt;
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index c575250..2c3894b 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -97,6 +97,12 @@ struct rte_ipv4_hdr {
 
 #define	RTE_IPV4_HDR_OFFSET_UNITS	8
 
+/* IPv4 options */
+#define RTE_IPV4_HDR_OPT_EOL     0
+#define RTE_IPV4_HDR_OPT_NOP       1
+#define RTE_IPV4_HDR_OPT_COPIED(v) ((v) & 0x80)
+#define RTE_IPV4_HDR_OPT_MAX_LEN   40
+
 /*
  * IPv4 address types
  */
-- 
1.8.3.1

RE: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Ananyev, Konstantin]

> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 1.8.3.1

Re:RE: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

Hi Konstantin,
This patch has been around for a long time, so what's next?
Huichao,Cai
At 2022-04-15 16:29:10, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>> According to RFC791,the options may appear or not in datagrams.
>> They must be implemented by all IP modules (host and gateways).
>> What is optional is their transmission in any particular datagram,
>> not their implementation.So we have to deal with it during the
>> fragmenting process.Add some test data for the IPv4 header optional
>> field fragmenting.
>> 
>> Signed-off-by: Huichao Cai <chcchc88@163.com>
>> ---
>
>Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
>> 1.8.3.1

[-- Attachment #2: Type: text/html, Size: 1341 bytes --]

Re:RE: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

Hi Konstantin,
This patch has been around for a long time, so what's next?
Huichao,Cai
At 2022-04-15 16:29:10, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>> According to RFC791,the options may appear or not in datagrams.
>> They must be implemented by all IP modules (host and gateways).
>> What is optional is their transmission in any particular datagram,
>> not their implementation.So we have to deal with it during the
>> fragmenting process.Add some test data for the IPv4 header optional
>> field fragmenting.
>> 
>> Signed-off-by: Huichao Cai <chcchc88@163.com>
>> ---
>
>Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
>> 1.8.3.1





 

[-- Attachment #2: Type: text/html, Size: 1397 bytes --]

Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Konstantin Ananyev]

Hi  Huichao,


> Hi Konstantin,
> This patch has been around for a long time, so what's next?

I acked it, which means that I am ok with that patch to go in.
Now it is up to main tree maintainers to pull it in.
Konstantin

> Huichao,Cai
> 
> At 2022-04-15 16:29:10, "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>>> According to RFC791,the options may appear or not in datagrams.
>>> They must be implemented by all IP modules (host and gateways).
>>> What is optional is their transmission in any particular datagram,
>>> not their implementation.So we have to deal with it during the
>>> fragmenting process.Add some test data for the IPv4 header optional
>>> field fragmenting.
>>> 
>>> Signed-off-by: Huichao Cai <chcchc88@163.com>
>>> ---
>>
>>Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>
>>> 1.8.3.1
> 
> 
> 
> 
> 

Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Thomas Monjalon]

15/04/2022 10:29, Ananyev, Konstantin:
> > According to RFC791,the options may appear or not in datagrams.
> > They must be implemented by all IP modules (host and gateways).
> > What is optional is their transmission in any particular datagram,
> > not their implementation.So we have to deal with it during the
> > fragmenting process.Add some test data for the IPv4 header optional
> > field fragmenting.
> > 
> > Signed-off-by: Huichao Cai <chcchc88@163.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks.

Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[David Marchand]

On Fri, Apr 15, 2022 at 5:27 AM Huichao Cai <chcchc88@163.com> wrote:
>
> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
>
> Signed-off-by: Huichao Cai <chcchc88@163.com>

gcc-12 raises warnings on both the unit test code and the library code.
See below.

> ---
>  app/test/test_ipfrag.c               | 219 ++++++++++++++++++++++++++++++++---
>  lib/ip_frag/rte_ipv4_fragmentation.c |  70 ++++++++++-
>  lib/net/rte_ip.h                     |   6 +
>  3 files changed, 272 insertions(+), 23 deletions(-)
>
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> index 1ced25a..610a86b 100644
> --- a/app/test/test_ipfrag.c
> +++ b/app/test/test_ipfrag.c
> @@ -18,10 +18,50 @@
>  #define NUM_MBUFS 128
>  #define BURST 32
>
> +uint8_t expected_first_frag_ipv4_opts_copied[] = {
> +       0x07, 0x0b, 0x04, 0x00,
> +       0x00, 0x00, 0x00, 0x00,
> +       0x00, 0x00, 0x00, 0x83,
> +       0x07, 0x04, 0xc0, 0xa8,
> +       0xe3, 0x96, 0x00, 0x00,
> +};
> +
> +uint8_t expected_sub_frag_ipv4_opts_copied[] = {
> +       0x83, 0x07, 0x04, 0xc0,
> +       0xa8, 0xe3, 0x96, 0x00,
> +};
> +
> +uint8_t expected_first_frag_ipv4_opts_nocopied[] = {
> +       0x07, 0x0b, 0x04, 0x00,
> +       0x00, 0x00, 0x00, 0x00,
> +       0x00, 0x00, 0x00, 0x00,
> +};
> +
> +uint8_t expected_sub_frag_ipv4_opts_nocopied[0];
> +
> +struct test_opt_data {
> +       bool is_first_frag;              /**< offset is 0 */
> +       bool opt_copied;                 /**< ip option copied flag */
> +       uint16_t len;                    /**< option data len */
> +       uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
> +};
> +
>  static struct rte_mempool *pkt_pool,
>                           *direct_pool,
>                           *indirect_pool;
>
> +static inline void
> +hex_to_str(uint8_t *hex, uint16_t len, char *str)
> +{
> +       int i;
> +
> +       for (i = 0; i < len; i++) {
> +               sprintf(str, "%02x", hex[i]);
> +               str += 2;
> +       }
> +       *str = 0;
> +}
> +
>  static int
>  setup_buf_pool(void)
>  {
> @@ -88,23 +128,67 @@ static void ut_teardown(void)
>  {
>  }
>
> +static inline void
> +test_get_ipv4_opt(bool is_first_frag, bool opt_copied,
> +       struct test_opt_data *expected_opt)
> +{
> +       if (is_first_frag) {
> +               if (opt_copied) {
> +                       expected_opt->len =
> +                               sizeof(expected_first_frag_ipv4_opts_copied);
> +                       rte_memcpy(expected_opt->data,
> +                               expected_first_frag_ipv4_opts_copied,
> +                               sizeof(expected_first_frag_ipv4_opts_copied));
> +               } else {
> +                       expected_opt->len =
> +                               sizeof(expected_first_frag_ipv4_opts_nocopied);
> +                       rte_memcpy(expected_opt->data,
> +                               expected_first_frag_ipv4_opts_nocopied,
> +                               sizeof(expected_first_frag_ipv4_opts_nocopied));
> +               }
> +       } else {
> +               if (opt_copied) {
> +                       expected_opt->len =
> +                               sizeof(expected_sub_frag_ipv4_opts_copied);
> +                       rte_memcpy(expected_opt->data,
> +                               expected_sub_frag_ipv4_opts_copied,
> +                               sizeof(expected_sub_frag_ipv4_opts_copied));
> +               } else {
> +                       expected_opt->len =
> +                               sizeof(expected_sub_frag_ipv4_opts_nocopied);
> +                       rte_memcpy(expected_opt->data,
> +                               expected_sub_frag_ipv4_opts_nocopied,
> +                               sizeof(expected_sub_frag_ipv4_opts_nocopied));
> +               }
> +       }
> +}
> +
>  static void
> -v4_allocate_packet_of(struct rte_mbuf *b, int fill,
> -                     size_t s, int df, uint8_t mf, uint16_t off,
> -                     uint8_t ttl, uint8_t proto, uint16_t pktid)
> +v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s,
> +       int df, uint8_t mf, uint16_t off, uint8_t ttl, uint8_t proto,
> +       uint16_t pktid, bool have_opt, bool is_first_frag, bool opt_copied)
>  {
>         /* Create a packet, 2k bytes long */
>         b->data_off = 0;
>         char *data = rte_pktmbuf_mtod(b, char *);
> -       rte_be16_t fragment_offset = 0; /**< fragmentation offset */
> +       rte_be16_t fragment_offset = 0; /* fragmentation offset */
> +       uint16_t iph_len;
> +       struct test_opt_data opt;
> +
> +       opt.len = 0;
> +
> +       if (have_opt)
> +               test_get_ipv4_opt(is_first_frag, opt_copied, &opt);


FAILED: app/test/dpdk-test.p/test_ipfrag.c.o
ccache gcc -Iapp/test/dpdk-test.p -Iapp/test -I../app/test -I. -I..
-Iconfig -I../config -Ilib/eal/include -I../lib/eal/include
-Ilib/eal/linux/include -I../lib/eal/linux/include
-Ilib/eal/x86/include -I../lib/eal/x86/include -Ilib/kvargs
-I../lib/kvargs -Ilib/metrics -I../lib/metrics -Ilib/telemetry
-I../lib/telemetry -Ilib/eal/common -I../lib/eal/common -Ilib/eal
-I../lib/eal -Ilib/ring -I../lib/ring -Ilib/rcu -I../lib/rcu
-Ilib/mempool -I../lib/mempool -Ilib/mbuf -I../lib/mbuf -Ilib/net
-I../lib/net -Ilib/meter -I../lib/meter -Ilib/ethdev -I../lib/ethdev
-Ilib/pci -I../lib/pci -Ilib/cmdline -I../lib/cmdline -Ilib/hash
-I../lib/hash -Ilib/timer -I../lib/timer -Ilib/acl -I../lib/acl
-Ilib/bbdev -I../lib/bbdev -Ilib/bitratestats -I../lib/bitratestats
-Ilib/bpf -I../lib/bpf -Ilib/cfgfile -I../lib/cfgfile
-Ilib/compressdev -I../lib/compressdev -Ilib/cryptodev
-I../lib/cryptodev -Ilib/distributor -I../lib/distributor -Ilib/efd
-I../lib/efd -Ilib/eventdev -I../lib/eventdev -Ilib/gpudev
-I../lib/gpudev -Ilib/gro -I../lib/gro -Ilib/gso -I../lib/gso
-Ilib/ip_frag -I../lib/ip_frag -Ilib/jobstats -I../lib/jobstats
-Ilib/kni -I../lib/kni -Ilib/latencystats -I../lib/latencystats
-Ilib/lpm -I../lib/lpm -Ilib/member -I../lib/member -Ilib/pcapng
-I../lib/pcapng -Ilib/power -I../lib/power -Ilib/rawdev
-I../lib/rawdev -Ilib/regexdev -I../lib/regexdev -Ilib/dmadev
-I../lib/dmadev -Ilib/rib -I../lib/rib -Ilib/reorder -I../lib/reorder
-Ilib/sched -I../lib/sched -Ilib/security -I../lib/security
-Ilib/stack -I../lib/stack -Ilib/vhost -I../lib/vhost -Ilib/ipsec
-I../lib/ipsec -Ilib/fib -I../lib/fib -Ilib/port -I../lib/port
-Ilib/pdump -I../lib/pdump -Ilib/table -I../lib/table -Ilib/pipeline
-I../lib/pipeline -Ilib/flow_classify -I../lib/flow_classify
-Ilib/graph -I../lib/graph -Ilib/node -I../lib/node -Idrivers/bus/pci
-I../drivers/bus/pci -I../drivers/bus/pci/linux -Idrivers/bus/vdev
-I../drivers/bus/vdev -Idrivers/mempool/ring -I../drivers/mempool/ring
-Idrivers/mempool/stack -I../drivers/mempool/stack
-Idrivers/event/skeleton -I../drivers/event/skeleton
-Idrivers/net/bonding -I../drivers/net/bonding -Idrivers/net/ring
-I../drivers/net/ring -Idrivers/net/null -I../drivers/net/null
-Idrivers/crypto/scheduler -I../drivers/crypto/scheduler
-fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch
-Wextra -Werror -O3 -include rte_config.h -Wcast-qual -Wdeprecated
-Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations
-Wmissing-prototypes -Wnested-externs -Wold-style-definition
-Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef
-Wwrite-strings -Wno-address-of-packed-member -Wno-packed-not-aligned
-Wno-missing-field-initializers -Wno-zero-length-bounds -D_GNU_SOURCE
-march=native -DALLOW_EXPERIMENTAL_API -Wno-format-truncation
-fno-strict-aliasing -DALLOW_INTERNAL_API -MD -MQ
app/test/dpdk-test.p/test_ipfrag.c.o -MF
app/test/dpdk-test.p/test_ipfrag.c.o.d -o
app/test/dpdk-test.p/test_ipfrag.c.o -c ../app/test/test_ipfrag.c
In file included from
/usr/lib/gcc/x86_64-redhat-linux/12/include/immintrin.h:43,
                 from
/usr/lib/gcc/x86_64-redhat-linux/12/include/x86intrin.h:32,
                 from ../lib/eal/x86/include/rte_vect.h:31,
                 from ../lib/eal/x86/include/rte_memcpy.h:17,
                 from ../lib/mempool/rte_mempool.h:46,
                 from ../lib/mbuf/rte_mbuf.h:38,
                 from ../lib/net/rte_ip.h:32,
                 from ../app/test/test_ipfrag.c:12:
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:369:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript ‘__m256i_u[1]’ is partly outside array bounds of
‘struct test_opt_data[1]’ [-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:187:30: note: at offset 36 into object ‘opt’
of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:370:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript 2 is outside array bounds of ‘struct test_opt_data[1]’
[-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:187:30: note: at offset 68 into object ‘opt’
of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:371:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript 3 is outside array bounds of ‘struct test_opt_data[1]’
[-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:187:30: note: at offset 100 into object
‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_mov64’ at ../lib/eal/x86/include/rte_memcpy.h:358:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:452:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript ‘__m256i_u[1]’ is partly outside array bounds of
‘const void[44]’ [-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:57:17: note: at offset 36 into object ‘data’
of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
../app/test/test_ipfrag.c:57:17: note: at offset [37, 40] into object
‘data’ of size 40
../app/test/test_ipfrag.c:187:30: note: at offset 168 into object
‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
../app/test/test_ipfrag.c:57:17: note: at offset 36 into object ‘data’
of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:457:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript [2, 2051] is outside array bounds of ‘const void[44]’
[-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:57:17: note: at offset [4, 40] into object
‘data’ of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object
‘data’ of size 40
../app/test/test_ipfrag.c:187:30: note: at offset [136, 200] into
object ‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
../app/test/test_ipfrag.c:57:17: note: at offset [4, 40] into object
‘data’ of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object
‘data’ of size 40
../app/test/test_ipfrag.c:187:30: note: at offset [136, 200] into
object ‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
../app/test/test_ipfrag.c:57:17: note: at offset [4, 40] into object
‘data’ of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:458:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript [2, 2052] is outside array bounds of ‘const void[44]’
[-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object
‘data’ of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
../app/test/test_ipfrag.c:57:17: note: at offset [6, 40] into object
‘data’ of size 40
../app/test/test_ipfrag.c:187:30: note: at offset [137, 201] into
object ‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object
‘data’ of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
../app/test/test_ipfrag.c:57:17: note: at offset [6, 40] into object
‘data’ of size 40
../app/test/test_ipfrag.c:187:30: note: at offset [137, 201] into
object ‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object
‘data’ of size 40
   57 |         uint8_t data[RTE_IPV4_HDR_OPT_MAX_LEN]; /**< option data */
      |                 ^~~~
In function ‘_mm256_loadu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:346:9,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:438:3,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘v4_allocate_packet_of’ at ../app/test/test_ipfrag.c:230:2,
    inlined from ‘test_ip_frag’ at ../app/test/test_ipfrag.c:402:4:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:929:10: error:
array subscript ‘__m256i_u[0]’ is partly outside array bounds of
‘struct test_opt_data[1]’ [-Werror=array-bounds]
  929 |   return *__P;
      |          ^~~~
../app/test/test_ipfrag.c: In function ‘test_ip_frag’:
../app/test/test_ipfrag.c:187:30: note: at offset [21, 36] into object
‘opt’ of size 44
  187 |         struct test_opt_data opt;
      |                              ^~~
cc1: all warnings being treated as errors
ninja: build stopped: subcommand failed.


>
> -       memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
> +       iph_len = sizeof(struct rte_ipv4_hdr) + opt.len;
> +       memset(data, fill, iph_len + s);
>
>         struct rte_ipv4_hdr *hdr = (struct rte_ipv4_hdr *)data;
>
> -       hdr->version_ihl = 0x45; /* standard IP header... */
> +       hdr->version_ihl = 0x40; /* ipv4 */
> +       hdr->version_ihl += (iph_len / 4);
>         hdr->type_of_service = 0;
> -       b->pkt_len = s + sizeof(struct rte_ipv4_hdr);
> +       b->pkt_len = s + iph_len;
>         b->data_len = b->pkt_len;
>         hdr->total_length = rte_cpu_to_be_16(b->pkt_len);
>         hdr->packet_id = rte_cpu_to_be_16(pktid);
> @@ -131,6 +215,8 @@ static void ut_teardown(void)
>         hdr->hdr_checksum = 0;
>         hdr->src_addr = rte_cpu_to_be_32(0x8080808);
>         hdr->dst_addr = rte_cpu_to_be_32(0x8080404);
> +
> +       rte_memcpy(hdr + 1, opt.data, opt.len);
>  }
>
>  static void
> @@ -187,6 +273,45 @@ static void ut_teardown(void)
>         }
>  }
>
> +static inline void
> +test_get_frag_opt(struct rte_mbuf **mb, int32_t num,
> +       struct test_opt_data *opt, int ipv, bool opt_copied)
> +{
> +       int32_t i;
> +
> +       for (i = 0; i < num; i++) {
> +               if (ipv == 4) {
> +                       struct rte_ipv4_hdr *iph =
> +                           rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
> +                       uint16_t header_len = (iph->version_ihl &
> +                               RTE_IPV4_HDR_IHL_MASK) *
> +                               RTE_IPV4_IHL_MULTIPLIER;
> +                       uint16_t opt_len = header_len -
> +                               sizeof(struct rte_ipv4_hdr);
> +
> +                       opt->opt_copied = opt_copied;
> +
> +                       if ((rte_be_to_cpu_16(iph->fragment_offset) &
> +                                   RTE_IPV4_HDR_OFFSET_MASK) == 0)
> +                               opt->is_first_frag = true;
> +                       else
> +                               opt->is_first_frag = false;
> +
> +                       if (likely(opt_len <= RTE_IPV4_HDR_OPT_MAX_LEN)) {
> +                               char *iph_opt = rte_pktmbuf_mtod_offset(mb[i],
> +                                   char *, sizeof(struct rte_ipv4_hdr));
> +                               opt->len = opt_len;
> +                               rte_memcpy(opt->data, iph_opt, opt_len);
> +                       } else {
> +                               opt->len = RTE_IPV4_HDR_OPT_MAX_LEN;
> +                               memset(opt->data, RTE_IPV4_HDR_OPT_EOL,
> +                                   sizeof(opt->data));
> +                       }
> +                       opt++;
> +               }
> +       }
> +}
> +
>  static int
>  test_ip_frag(void)
>  {
> @@ -206,32 +331,52 @@ static void ut_teardown(void)
>                 uint16_t pkt_id;
>                 int      expected_frags;
>                 uint16_t expected_fragment_offset[BURST];
> +               bool have_opt;
> +               bool is_first_frag;
> +               bool opt_copied;
>         } tests[] = {
>                  {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> -                 {0x2000, 0x009D}},
> +                 {0x2000, 0x009D}, false},
>                  {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
> -                 {0x2000, 0x009D}},
> +                 {0x2000, 0x009D}, false},
>                  {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
> -                 {0x2000, 0x2048, 0x0090}},
> +                 {0x2000, 0x2048, 0x0090}, false},
>                  {4, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
>                  {4, 600, 1400, 1, 0, 0, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
>                  {4, 600, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,         3,
> -                 {0x2000, 0x2048, 0x0090}},
> +                 {0x2000, 0x2046, 0x008C}, true, true, true},
> +                /* The first fragment */
> +                {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           5,
> +                 {0x2000, 0x2003, 0x2006, 0x2009, 0x200C}, true, true, true},
> +                /* The middle fragment */
>                  {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
> -                 {0x200D, 0x2013, 0x2019}},
> -
> +                 {0x200D, 0x2012, 0x2017}, true, false, true},
> +                /* The last fragment */
> +                {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
> +                 {0x201A, 0x201F, 0x0024}, true, false, true},
> +                /* The first fragment */
> +                {4, 68, 104, 0, 1, 0, 0, IPPROTO_ICMP, RND_ID,           4,
> +                 {0x2000, 0x2004, 0x2008, 0x200C}, true, true, false},
> +                /* The middle fragment */
> +                {4, 68, 104, 0, 1, 13, 0, IPPROTO_ICMP, RND_ID,          3,
> +                 {0x200D, 0x2013, 0x2019}, true, false, false},
> +                /* The last fragment */
> +                {4, 68, 104, 0, 0, 26, 0, IPPROTO_ICMP, RND_ID,          3,
> +                 {0x201A, 0x2020, 0x0026}, true, false, false},
>                  {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> -                 {0x0001, 0x04D0}},
> +                 {0x0001, 0x04D0}, false},
>                  {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> -                 {0x0001, 0x04E0}},
> +                 {0x0001, 0x04E0}, false},
>                  {6, 4, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,    -EINVAL},
>                  {6, 1300, 1400, 0, 0, 0, 0, IPPROTO_ICMP, RND_ID,        2,
> -                 {0x0001, 0x04E0}},
> +                 {0x0001, 0x04E0}, false},
>         };
>
>         for (i = 0; i < RTE_DIM(tests); i++) {
>                 int32_t len = 0;
>                 uint16_t fragment_offset[BURST];
> +               struct test_opt_data opt_res[BURST];
> +               struct test_opt_data opt_exp;
>                 uint16_t pktid = tests[i].pkt_id;
>                 struct rte_mbuf *pkts_out[BURST];
>                 struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> @@ -250,7 +395,10 @@ static void ut_teardown(void)
>                                               tests[i].set_of,
>                                               tests[i].ttl,
>                                               tests[i].proto,
> -                                             pktid);
> +                                             pktid,
> +                                             tests[i].have_opt,
> +                                             tests[i].is_first_frag,
> +                                             tests[i].opt_copied);
>                 } else if (tests[i].ipv == 6) {
>                         v6_allocate_packet_of(b, 0x41414141,
>                                               tests[i].pkt_size,
> @@ -275,17 +423,20 @@ static void ut_teardown(void)
>                 if (len > 0) {
>                         test_get_offset(pkts_out, len,
>                             fragment_offset, tests[i].ipv);
> +                       if (tests[i].have_opt)
> +                               test_get_frag_opt(pkts_out, len, opt_res,
> +                                       tests[i].ipv, tests[i].opt_copied);
>                         test_free_fragments(pkts_out, len);
>                 }
>
> -               printf("%zd: checking %d with %d\n", i, len,
> +               printf("[check frag number]%zd: checking %d with %d\n", i, len,
>                        tests[i].expected_frags);
>                 RTE_TEST_ASSERT_EQUAL(len, tests[i].expected_frags,
>                                       "Failed case %zd.\n", i);
>
>                 if (len > 0) {
>                         for (j = 0; j < (size_t)len; j++) {
> -                               printf("%zd-%zd: checking %d with %d\n",
> +                               printf("[check offset]%zd-%zd: checking %d with %d\n",
>                                     i, j, fragment_offset[j],
>                                     rte_cpu_to_be_16(
>                                         tests[i].expected_fragment_offset[j]));
> @@ -294,6 +445,36 @@ static void ut_teardown(void)
>                                         tests[i].expected_fragment_offset[j]),
>                                     "Failed case %zd.\n", i);
>                         }
> +
> +                       if (tests[i].have_opt && (tests[i].ipv == 4)) {
> +                               for (j = 0; j < (size_t)len; j++) {
> +                                       char opt_res_str[2 *
> +                                               RTE_IPV4_HDR_OPT_MAX_LEN + 1];
> +                                       char opt_exp_str[2 *
> +                                               RTE_IPV4_HDR_OPT_MAX_LEN + 1];
> +
> +                                       test_get_ipv4_opt(
> +                                               opt_res[j].is_first_frag,
> +                                               opt_res[j].opt_copied,
> +                                               &opt_exp);
> +                                       hex_to_str(opt_res[j].data,
> +                                               opt_res[j].len,
> +                                               opt_res_str);
> +                                       hex_to_str(opt_exp.data,
> +                                               opt_exp.len,
> +                                               opt_exp_str);
> +
> +                                       printf(
> +                                               "[check ipv4 option]%zd-%zd: checking (len:%u)%s with (len:%u)%s\n",
> +                                               i, j,
> +                                               opt_res[j].len, opt_res_str,
> +                                               opt_exp.len, opt_exp_str);
> +                                               RTE_TEST_ASSERT_SUCCESS(
> +                                                       strcmp(opt_res_str,
> +                                                               opt_exp_str),
> +                                               "Failed case %zd.\n", i);
> +                               }
> +                       }
>                 }
>
>         }
> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index 2e7739d..a562424 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -22,6 +22,8 @@
>
>  #define        IPV4_HDR_FO_ALIGN                       (1 << RTE_IPV4_HDR_FO_SHIFT)
>
> +#define IPV4_HDR_MAX_LEN                       60
> +
>  static inline void __fill_ipv4hdr_frag(struct rte_ipv4_hdr *dst,
>                 const struct rte_ipv4_hdr *src, uint16_t header_len,
>                 uint16_t len, uint16_t fofs, uint16_t dofs, uint32_t mf)
> @@ -41,6 +43,49 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>                 rte_pktmbuf_free(mb[i]);
>  }
>
> +static inline uint16_t __create_ipopt_frag_hdr(uint8_t *iph,
> +       uint16_t ipopt_len, uint8_t *ipopt_frag_hdr)
> +{
> +       uint16_t len = ipopt_len;
> +       struct rte_ipv4_hdr *iph_opt = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +
> +       ipopt_len = 0;
> +       rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> +       ipopt_frag_hdr += sizeof(struct rte_ipv4_hdr);
> +
> +       uint8_t *p_opt = iph + sizeof(struct rte_ipv4_hdr);
> +
> +       while (len > 0) {
> +               if (unlikely(*p_opt == RTE_IPV4_HDR_OPT_NOP)) {
> +                       len--;
> +                       p_opt++;
> +                       continue;
> +               } else if (unlikely(*p_opt == RTE_IPV4_HDR_OPT_EOL))
> +                       break;
> +
> +               if (unlikely(p_opt[1] < 2 || p_opt[1] > len))
> +                       break;
> +
> +               if (RTE_IPV4_HDR_OPT_COPIED(*p_opt)) {
> +                       rte_memcpy(ipopt_frag_hdr + ipopt_len,
> +                               p_opt, p_opt[1]);
> +                       ipopt_len += p_opt[1];
> +               }
> +
> +               len -= p_opt[1];
> +               p_opt += p_opt[1];
> +       }
> +
> +       len = RTE_ALIGN_CEIL(ipopt_len, RTE_IPV4_IHL_MULTIPLIER);
> +       memset(ipopt_frag_hdr + ipopt_len,
> +               RTE_IPV4_HDR_OPT_EOL, len - ipopt_len);
> +       ipopt_len = len;
> +       iph_opt->ihl = (sizeof(struct rte_ipv4_hdr) + ipopt_len) /
> +               RTE_IPV4_IHL_MULTIPLIER;
> +
> +       return ipopt_len;
> +}
> +
>  /**
>   * IPv4 fragmentation.
>   *
> @@ -76,6 +121,8 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>         uint32_t more_in_segs;
>         uint16_t fragment_offset, flag_offset, frag_size, header_len;
>         uint16_t frag_bytes_remaining;
> +       uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
> +       uint16_t ipopt_len;
>
>         /*
>          * Formal parameter checking.
> @@ -118,6 +165,10 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>         out_pkt_pos = 0;
>         fragment_offset = 0;
>
> +       ipopt_len = header_len - sizeof(struct rte_ipv4_hdr);
> +       if (unlikely(ipopt_len > RTE_IPV4_HDR_OPT_MAX_LEN))
> +               return -EINVAL;
> +
>         more_in_segs = 1;
>         while (likely(more_in_segs)) {
>                 struct rte_mbuf *out_pkt = NULL, *out_seg_prev = NULL;
> @@ -188,10 +239,21 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>                     (uint16_t)out_pkt->pkt_len,
>                     flag_offset, fragment_offset, more_in_segs);
>
> -               fragment_offset = (uint16_t)(fragment_offset +
> -                   out_pkt->pkt_len - header_len);
> -
> -               out_pkt->l3_len = header_len;
> +               if (unlikely((fragment_offset == 0) && (ipopt_len) &&
> +                           ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))) {
> +                       ipopt_len = __create_ipopt_frag_hdr((uint8_t *)in_hdr,
> +                               ipopt_len, ipopt_frag_hdr);
> +                       fragment_offset = (uint16_t)(fragment_offset +
> +                               out_pkt->pkt_len - header_len);
> +                       out_pkt->l3_len = header_len;
> +
> +                       header_len = sizeof(struct rte_ipv4_hdr) + ipopt_len;
> +                       in_hdr = (struct rte_ipv4_hdr *)ipopt_frag_hdr;
> +               } else {
> +                       fragment_offset = (uint16_t)(fragment_offset +
> +                               out_pkt->pkt_len - header_len);
> +                       out_pkt->l3_len = header_len;
> +               }

FAILED: lib/librte_ip_frag.a.p/ip_frag_rte_ipv4_fragmentation.c.o
ccache gcc -Ilib/librte_ip_frag.a.p -Ilib -I../lib -Ilib/ip_frag
-I../lib/ip_frag -I. -I.. -Iconfig -I../config -Ilib/eal/include
-I../lib/eal/include -Ilib/eal/linux/include
-I../lib/eal/linux/include -Ilib/eal/x86/include
-I../lib/eal/x86/include -Ilib/eal/common -I../lib/eal/common
-Ilib/eal -I../lib/eal -Ilib/kvargs -I../lib/kvargs -Ilib/metrics
-I../lib/metrics -Ilib/telemetry -I../lib/telemetry -Ilib/ethdev
-I../lib/ethdev -Ilib/net -I../lib/net -Ilib/mbuf -I../lib/mbuf
-Ilib/mempool -I../lib/mempool -Ilib/ring -I../lib/ring -Ilib/meter
-I../lib/meter -Ilib/hash -I../lib/hash -Ilib/rcu -I../lib/rcu
-fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch
-Wextra -Werror -O3 -g -include rte_config.h -Wcast-qual -Wdeprecated
-Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations
-Wmissing-prototypes -Wnested-externs -Wold-style-definition
-Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef
-Wwrite-strings -Wno-address-of-packed-member -Wno-packed-not-aligned
-Wno-missing-field-initializers -Wno-zero-length-bounds -D_GNU_SOURCE
-fPIC -march=native -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API
-Wno-format-truncation -DRTE_LOG_DEFAULT_LOGTYPE=lib.ip_frag -MD -MQ
lib/librte_ip_frag.a.p/ip_frag_rte_ipv4_fragmentation.c.o -MF
lib/librte_ip_frag.a.p/ip_frag_rte_ipv4_fragmentation.c.o.d -o
lib/librte_ip_frag.a.p/ip_frag_rte_ipv4_fragmentation.c.o -c
../lib/ip_frag/rte_ipv4_fragmentation.c
In file included from
/usr/lib/gcc/x86_64-redhat-linux/12/include/immintrin.h:43,
                 from
/usr/lib/gcc/x86_64-redhat-linux/12/include/x86intrin.h:32,
                 from ../lib/eal/x86/include/rte_vect.h:31,
                 from ../lib/eal/x86/include/rte_memcpy.h:17,
                 from ../lib/ip_frag/rte_ipv4_fragmentation.c:8:
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:369:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript ‘__m256i_u[1]’ is partly outside array bounds of
‘uint8_t[60]’ {aka ‘unsigned char[60]’} [-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [52,
60] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:370:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript [2, 3] is outside array bounds of ‘uint8_t[60]’ {aka
‘unsigned char[60]’} [-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [84,
124] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov128’ at ../lib/eal/x86/include/rte_memcpy.h:371:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:445:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript [3, 4] is outside array bounds of ‘uint8_t[60]’ {aka
‘unsigned char[60]’} [-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [116,
156] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_mov64’ at ../lib/eal/x86/include/rte_memcpy.h:358:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:452:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript ‘__m256i_u[1]’ is partly outside array bounds of
‘void[60]’ [-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [180,
240] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [52,
60] into object ‘ipopt_frag_hdr’ of size 60
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:457:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript [2, 7] is outside array bounds of ‘void[60]’
[-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [148,
272] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [148,
272] into object ‘ipopt_frag_hdr’ of size 60
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [20,
60] into object ‘ipopt_frag_hdr’ of size 60
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:458:4,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript [2, 8] is outside array bounds of ‘void[60]’
[-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [149,
273] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [149,
273] into object ‘ipopt_frag_hdr’ of size 60
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [21,
60] into object ‘ipopt_frag_hdr’ of size 60
In function ‘_mm256_storeu_si256’,
    inlined from ‘rte_mov32’ at ../lib/eal/x86/include/rte_memcpy.h:347:2,
    inlined from ‘rte_memcpy_generic’ at
../lib/eal/x86/include/rte_memcpy.h:438:3,
    inlined from ‘rte_memcpy’ at ../lib/eal/x86/include/rte_memcpy.h:851:10,
    inlined from ‘__create_ipopt_frag_hdr’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:68:4,
    inlined from ‘rte_ipv4_fragment_packet’ at
../lib/ip_frag/rte_ipv4_fragmentation.c:242:16:
/usr/lib/gcc/x86_64-redhat-linux/12/include/avxintrin.h:935:8: error:
array subscript ‘__m256i_u[1]’ is partly outside array bounds of
‘uint8_t[60]’ {aka ‘unsigned char[60]’} [-Werror=array-bounds]
  935 |   *__P = __A;
      |   ~~~~~^~~~~
../lib/ip_frag/rte_ipv4_fragmentation.c: In function ‘rte_ipv4_fragment_packet’:
../lib/ip_frag/rte_ipv4_fragmentation.c:122:17: note: at offset [37,
60] into object ‘ipopt_frag_hdr’ of size 60
  122 |         uint8_t ipopt_frag_hdr[IPV4_HDR_MAX_LEN];
      |                 ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors



-- 
David Marchand

Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Stephen Hemminger]

On Thu, 16 Jun 2022 17:10:46 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Fri, Apr 15, 2022 at 5:27 AM Huichao Cai <chcchc88@163.com> wrote:
> >
> > According to RFC791,the options may appear or not in datagrams.
> > They must be implemented by all IP modules (host and gateways).
> > What is optional is their transmission in any particular datagram,
> > not their implementation.So we have to deal with it during the
> > fragmenting process.Add some test data for the IPv4 header optional
> > field fragmenting.
> >
> > Signed-off-by: Huichao Cai <chcchc88@163.com>  
> 
> gcc-12 raises warnings on both the unit test code and the library code.
> See below.

Since the copies will all be short why bother using rte_memcpy() all over
the place.  Especially in the test code, just use memcpy().

Re:Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]

Hi,Stephen


There are some things I don't quite understand.Hope you can answer that.
This will help me avoid similar errors in subsequent patch submissions.Thanks!


There are places where rte_memcpy functions are used：
============================================
In test_ipfrag.c：
from func test_get_ipv4_opt: 
rte_memcpy(expected_opt->data,expected_first_frag_ipv4_opts_copied,sizeof(expected_first_frag_ipv4_opts_copied));
rte_memcpy(expected_opt>data,expected_first_frag_ipv4_opts_nocopied,sizeof(expected_first_frag_ipv4_opts_nocopied));
rte_memcpy(expected_opt->data,expected_sub_frag_ipv4_opts_copied,sizeof(expected_sub_frag_ipv4_opts_copied));
rte_memcpy(expected_opt->data,expected_sub_frag_ipv4_opts_nocopied,sizeof(expected_sub_frag_ipv4_opts_nocopied));
from func v4_allocate_packet_of:
rte_memcpy(hdr + 1, opt.data, opt.len);
from func test_get_frag_opt:
rte_memcpy(opt->data, iph_opt, opt_len);


In rte_ipv4_fragmentation.c:
from func v4_allocate_packet_of:
rte_memcpy(dst, src, header_len);
from func __create_ipopt_frag_hdr:

rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
rte_memcpy(ipopt_frag_hdr + ipopt_len, p_opt, p_opt[1]);
============================================


These are the compilation errors:
============================================
test_ipfrag.c:230
In test_ipfrag.c：
from func v4_allocate_packet_of:
rte_memcpy(hdr + 1, opt.data, opt.len);
rte_ipv4_fragmentation.c:68
In rte_ipv4_fragmentation.c:
from func __create_ipopt_frag_hdr:
rte_memcpy(ipopt_frag_hdr + ipopt_len, p_opt, p_opt[1]);
============================================


1.Do I need to replace all rte_memcpy with memcpy or only the two rte_memcpy that compile the error are replaced by memcpy?
2.
>Since the copies will all be short why bother using rte_memcpy() all over
>the place.  Especially in the test code, just use memcpy().
For example,in app/test-pmd/cmdline.c:from func cmd_set_vxlan_parsed:rte_memcpy(vxlan_encap_conf.vni, &id.vni[1], 3);Why this place can be used rte_memcpy?
3.For example, how such a compilation error occurs:
../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object‘data’ of size 40
4.Under what circumstances can we use rte_memcpy?


Huichao,Cai

[-- Attachment #2: Type: text/html, Size: 9136 bytes --]

Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Stephen Hemminger]

On Fri, 17 Jun 2022 11:52:25 +0800 (CST)
"Huichao Cai" <chcchc88@163.com> wrote:

> Hi,Stephen
> 
> 
> There are some things I don't quite understand.Hope you can answer that.
> This will help me avoid similar errors in subsequent patch submissions.Thanks!
> 
> 
> There are places where rte_memcpy functions are used：
> ============================================
> In test_ipfrag.c：
> from func test_get_ipv4_opt: 
> rte_memcpy(expected_opt->data,expected_first_frag_ipv4_opts_copied,sizeof(expected_first_frag_ipv4_opts_copied));
> rte_memcpy(expected_opt>data,expected_first_frag_ipv4_opts_nocopied,sizeof(expected_first_frag_ipv4_opts_nocopied));  
> rte_memcpy(expected_opt->data,expected_sub_frag_ipv4_opts_copied,sizeof(expected_sub_frag_ipv4_opts_copied));
> rte_memcpy(expected_opt->data,expected_sub_frag_ipv4_opts_nocopied,sizeof(expected_sub_frag_ipv4_opts_nocopied));
> from func v4_allocate_packet_of:
> rte_memcpy(hdr + 1, opt.data, opt.len);
> from func test_get_frag_opt:
> rte_memcpy(opt->data, iph_opt, opt_len);
> 
> 
> In rte_ipv4_fragmentation.c:
> from func v4_allocate_packet_of:
> rte_memcpy(dst, src, header_len);
> from func __create_ipopt_frag_hdr:
> 
> rte_memcpy(ipopt_frag_hdr, iph, sizeof(struct rte_ipv4_hdr));
> rte_memcpy(ipopt_frag_hdr + ipopt_len, p_opt, p_opt[1]);
> ============================================
> 
> 
> These are the compilation errors:
> ============================================
> test_ipfrag.c:230
> In test_ipfrag.c：
> from func v4_allocate_packet_of:
> rte_memcpy(hdr + 1, opt.data, opt.len);
> rte_ipv4_fragmentation.c:68
> In rte_ipv4_fragmentation.c:
> from func __create_ipopt_frag_hdr:
> rte_memcpy(ipopt_frag_hdr + ipopt_len, p_opt, p_opt[1]);
> ============================================
> 
> 
> 1.Do I need to replace all rte_memcpy with memcpy or only the two rte_memcpy that compile the error are replaced by memcpy?

I would just replace all of the rte_memcpy with memcpy

> 2.
> >Since the copies will all be short why bother using rte_memcpy() all over
> >the place.  Especially in the test code, just use memcpy().  
> For example,in app/test-pmd/cmdline.c:from func cmd_set_vxlan_parsed:rte_memcpy(vxlan_encap_conf.vni, &id.vni[1], 3);Why this place can be used rte_memcpy?
> 3.For example, how such a compilation error occurs:
> ../app/test/test_ipfrag.c:57:17: note: at offset [5, 40] into object‘data’ of size 40
> 4.Under what circumstances can we use rte_memcpy?


It depends. The recommendation here was that fixing warnings is higher priority that saving a few cycles
in an underutilized part of DPDK.

Rte_memcpy() was added in early versions of DPDK because the standard toolchain gcc/glibc
was not using the optimum set of instructions on x86.  Rather than fix glibc, Intel wrote
their own rte_memcpy(). Then DPDK developers, started to assume that rte_memcpy() is always best.

I expect that rte_memcpy() is able to do better than memcpy() for larger copies because it is
likely to use bigger vector instructions and check for alignment.
For small copies just doing the mov's directly is going to be as fast or faster.
In fact, lots of places in DPDK should
replace rte_memcpy() with simple structure assignment to preserve type safety.

This is somewhat historical data, it might be wrong. It would be worthwhile to have benchmarks
across different sizes (variable and fixed), different compilers, and different CPU's.
There might be surprising results.

Re:Re: [PATCH v7] ip_frag: add IPv4 options fragment and test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

Hi,Stephen
Thank you very much for your reply!
>I would just replace all of the rte_memcpy with memcpy  
I will replace all of the rte_memcpy with memcpy.
>I expect that rte_memcpy() is able to do better than memcpy() for larger copies because it is
>likely to use bigger vector instructions and check for alignment.
>For small copies just doing the mov's directly is going to be as fast or faster.
>In fact, lots of places in DPDK should
>replace rte_memcpy() with simple structure assignment to preserve type safety.
I don't know the dividing line(the size of the data) between rte_memcpy and memcpy.
We simply test 1500 bytes of replication, memcpy seems to be faster, maybe our test is not accurate enough.
>This is somewhat historical data, it might be wrong. It would be worthwhile to have benchmarks
>across different sizes (variable and fixed), different compilers, and different CPU's.
>There might be surprising results.
So I hope this can go on and provide a more professional rte_memcpy manual.Thanks!
Huichao,Cai


[-- Attachment #2: Type: text/html, Size: 2071 bytes --]

Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Dariusz Sosnowski]

Hi,

On Thu, 2 Dec 2021 10:24:40 +0800, Huichao Cai wrote:
> > Substituting options with NOOP might cause rte_ipv4_fragment_packet to produce more fragments than necessary, since options with copied flag unset will still occupy space in IPv4 header.
> --The "ip_options_fragment" just make a replacement and doesn't change the length of the IPv4 header.So I don't quite understand why it leads to produce more fragments.
If options with copied flag unset are not copied, then IPv4 headers in the fragments (despite 1st fragment) will be shorter. This leaves more byte space for the payload and in effect fragmentation might produce less fragments.

Best regards,
Dariusz Sosnowski



 

Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Ananyev, Konstantin]

> 
> According to RFC791,the options may appear or not in datagrams.
> They must be implemented by all IP modules (host and gateways).
> What is optional is their transmission in any particular datagram,
> not their implementation.So we have to deal with it during the
> fragmenting process.Add some test data for the IPv4 header optional
> field fragmenting.
> 
> Signed-off-by: Huichao Cai <chcchc88@163.com>
> ---

....

> diff --git a/lib/ip_frag/rte_ipv4_fragmentation.c b/lib/ip_frag/rte_ipv4_fragmentation.c
> index 2e7739d..bcafa29 100644
> --- a/lib/ip_frag/rte_ipv4_fragmentation.c
> +++ b/lib/ip_frag/rte_ipv4_fragmentation.c
> @@ -1,4 +1,4 @@
> -/* SPDX-License-Identifier: BSD-3-Clause
> +/* SPDX-License-Identifier: (BSD-3-Clause OR GPL-2.0)
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
> 
> @@ -12,6 +12,13 @@
> 
>  #include "ip_frag_common.h"
> 
> +/* IP options */
> +#define RTE_IPOPT_COPY				0x80
> +#define RTE_IPOPT_CONTROL			0x00
> +#define RTE_IPOPT_END				(0 | RTE_IPOPT_CONTROL)
> +#define RTE_IPOPT_NOOP				(1 | RTE_IPOPT_CONTROL)
> +#define RTE_IPOPT_COPIED(o)			((o) & RTE_IPOPT_COPY)
> +
>  /* Fragment Offset */
>  #define	RTE_IPV4_HDR_DF_SHIFT			14
>  #define	RTE_IPV4_HDR_MF_SHIFT			13
> @@ -41,6 +48,38 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		rte_pktmbuf_free(mb[i]);
>  }
> 
> +/*
> + *	Options "fragmenting", just fill options not
> + *	allowed in fragments with NOOPs.
> + *	Simple and stupid 8), but the most efficient way.
> + */
> +static inline void ip_options_fragment(struct rte_ipv4_hdr *iph)
> +{
> +	unsigned char *optptr = (unsigned char *)iph +
> +	    sizeof(struct rte_ipv4_hdr);

As a nit, why not 'uint8_t *', to keep style the same through all file? 

> +	int l = (iph->version_ihl & RTE_IPV4_HDR_IHL_MASK) *
> +	    RTE_IPV4_IHL_MULTIPLIER - sizeof(struct rte_ipv4_hdr);

We already done such calculation in rte_ipv4_fragment_packet(),
so can re-use header_len value here.

> +	int optlen;
> +
> +	while (l > 0) {
> +		switch (*optptr) {
> +		case RTE_IPOPT_END:
> +			return;
> +		case RTE_IPOPT_NOOP:
> +			l--;
> +			optptr++;
> +			continue;
> +		}
> +		optlen = optptr[1];
> +		if (optlen < 2 || optlen > l)

Why optlen==1 is not considered as valid one?

> +			return;
> +		if (!RTE_IPOPT_COPIED(*optptr))
> +			memset(optptr, RTE_IPOPT_NOOP, optlen);
> +		l -= optlen;
> +		optptr += optlen;
> +	}
> +}
> +
>  /**
>   * IPv4 fragmentation.
>   *
> @@ -188,6 +227,17 @@ static inline void __free_fragments(struct rte_mbuf *mb[], uint32_t num)
>  		    (uint16_t)out_pkt->pkt_len,
>  		    flag_offset, fragment_offset, more_in_segs);
> 
> +		/*
> +		 * ANK:

What means 'ANK' here? 

> dirty, but effective trick. Upgrade options only if
> +		 * the segment to be fragmented was THE FIRST (otherwise,
> +		 * options are already fixed) and make it ONCE
> +		 * on the initial mbuf, so that all the following fragments
> +		 * will inherit fixed options.
> +		 */
> +		if ((fragment_offset == 0) &&
> +			    ((flag_offset & RTE_IPV4_HDR_OFFSET_MASK) == 0))
> +			ip_options_fragment(in_hdr);
> +

I see two problems with that approach:
- you modify incoming packet's header - which is the change in behaviour,
  and doesn't look right at all.
- you remove not-copied options from all fragments.
  As I can read RFC 791 - first fragment should have a copy of all options present
  in original packet, while other fragments need to have only those that have to be
  copied.  

>  		fragment_offset = (uint16_t)(fragment_offset +
>  		    out_pkt->pkt_len - header_len);
> 
> --
> 1.8.3.1

Re:Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Huichao Cai]

[-- Attachment #1: Type: text/plain, Size: 103 bytes --]

A small problem.Why is the content of the email just sent to you not visible at Patchwork (this patch).

[-- Attachment #2: Type: text/html, Size: 348 bytes --]

Re: [PATCH] ip_frag: add IPv4 options fragment and unit test data

[Ferruh Yigit]

On 2/11/2022 2:20 AM, Huichao Cai wrote:
> A small problem.Why is the content of the email just sent to you not visible at Patchwork (this patch).
> 
> 

Hi Huichao,

The discussion is not in the same email thread, it looks like it spread
into multiple threads.
This is because some email headers (References:, In-Reply-To:) are dropped
by some emails (email clients?) causing reply seen as a new thread.

Because of this missing email header, I think patchwork can't relate
discussion to original patch/thread.

for record patchwork:
https://patches.dpdk.org/project/dpdk/patch/1637743626-70632-1-git-send-email-chcchc88@163.com/