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

  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(-)
  

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
  

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