test/ipfrag: add test content to the test unit

  Add the test content of the fragment_offset(offset and MF)
to the test_ip_frag function. Add test data for a fragment
that is not the last fragment.

Signed-off-by: huichao cai <chcchc88@163.com>
---
 app/test/test_ipfrag.c | 95 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 79 insertions(+), 16 deletions(-)
  

> Add the test content of the fragment_offset(offset and MF)
> to the test_ip_frag function. Add test data for a fragment
> that is not the last fragment.
> 
> Signed-off-by: huichao cai <chcchc88@163.com>
> ---
>  app/test/test_ipfrag.c | 95 +++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 79 insertions(+), 16 deletions(-)
> 
> diff --git a/app/test/test_ipfrag.c b/app/test/test_ipfrag.c
> index da8c212..1ced25a 100644
> --- a/app/test/test_ipfrag.c
> +++ b/app/test/test_ipfrag.c
> @@ -89,12 +89,14 @@ static void ut_teardown(void)
>  }
> 
>  static void
> -v4_allocate_packet_of(struct rte_mbuf *b, int fill, size_t s, int df,
> +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)
>  {
>  	/* 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 */
> 
>  	memset(data, fill, sizeof(struct rte_ipv4_hdr) + s);
> 
> @@ -106,9 +108,17 @@ static void ut_teardown(void)
>  	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);
> -	hdr->fragment_offset = 0;
> +
>  	if (df)
> -		hdr->fragment_offset = rte_cpu_to_be_16(0x4000);
> +		fragment_offset |= 0x4000;
> +
> +	if (mf)
> +		fragment_offset |= 0x2000;
> +
> +	if (off)
> +		fragment_offset |= off;
> +
> +	hdr->fragment_offset = rte_cpu_to_be_16(fragment_offset);
> 
>  	if (!ttl)
>  		ttl = 64; /* default to 64 */
> @@ -155,38 +165,73 @@ static void ut_teardown(void)
>  		rte_pktmbuf_free(mb[i]);
>  }
> 
> +static inline void
> +test_get_offset(struct rte_mbuf **mb, int32_t len,
> +	uint16_t *offset, int ipv)
> +{
> +	int32_t i;
> +
> +	for (i = 0; i < len; i++) {
> +		if (ipv == 4) {
> +			struct rte_ipv4_hdr *iph =
> +			    rte_pktmbuf_mtod(mb[i], struct rte_ipv4_hdr *);
> +			offset[i] = iph->fragment_offset;
> +		} else if (ipv == 6) {
> +			struct ipv6_extension_fragment *fh =
> +			    rte_pktmbuf_mtod_offset(
> +					mb[i],
> +					struct ipv6_extension_fragment *,
> +					sizeof(struct rte_ipv6_hdr));
> +			offset[i] = fh->frag_data;
> +		}
> +	}
> +}
> +
>  static int
>  test_ip_frag(void)
>  {
>  	static const uint16_t RND_ID = UINT16_MAX;
>  	int result = TEST_SUCCESS;
> -	size_t i;
> +	size_t i, j;
> 
>  	struct test_ip_frags {
>  		int      ipv;
>  		size_t   mtu_size;
>  		size_t   pkt_size;
>  		int      set_df;
> +		uint8_t  set_mf;
> +		uint16_t set_of;
>  		uint8_t  ttl;
>  		uint8_t  proto;
>  		uint16_t pkt_id;
>  		int      expected_frags;
> +		uint16_t expected_fragment_offset[BURST];
>  	} tests[] = {
> -		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> -		     {4, 1280, 1400, 0, 64, IPPROTO_ICMP, 0,      2},
> -		     {4,  600, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 3},
> -		     {4,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
> -		     {4,  600, 1400, 1, 64, IPPROTO_ICMP, RND_ID, -ENOTSUP},
> -		     {4,  600, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 3},
> -
> -		     {6, 1280, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> -		     {6, 1300, 1400, 0, 64, IPPROTO_ICMP, RND_ID, 2},
> -		     {6,    4, 1400, 0, 64, IPPROTO_ICMP, RND_ID, -EINVAL},
> -		     {6, 1300, 1400, 0,  0, IPPROTO_ICMP, RND_ID, 2},
> +		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> +		  {0x2000, 0x009D}},
> +		 {4, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, 0,            2,
> +		  {0x2000, 0x009D}},
> +		 {4,  600, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       3,
> +		  {0x2000, 0x2048, 0x0090}},
> +		 {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}},
> +
> +		 {6, 1280, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> +		  {0x0001, 0x04D0}},
> +		 {6, 1300, 1400, 0, 0, 0, 64, IPPROTO_ICMP, RND_ID,       2,
> +		  {0x0001, 0x04E0}},
> +		 {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}},
>  	};
> 
>  	for (i = 0; i < RTE_DIM(tests); i++) {
>  		int32_t len = 0;
> +		uint16_t fragment_offset[BURST];
>  		uint16_t pktid = tests[i].pkt_id;
>  		struct rte_mbuf *pkts_out[BURST];
>  		struct rte_mbuf *b = rte_pktmbuf_alloc(pkt_pool);
> @@ -201,6 +246,8 @@ static void ut_teardown(void)
>  			v4_allocate_packet_of(b, 0x41414141,
>  					      tests[i].pkt_size,
>  					      tests[i].set_df,
> +					      tests[i].set_mf,
> +					      tests[i].set_of,
>  					      tests[i].ttl,
>  					      tests[i].proto,
>  					      pktid);
> @@ -225,14 +272,30 @@ static void ut_teardown(void)
> 
>  		rte_pktmbuf_free(b);
> 
> -		if (len > 0)
> +		if (len > 0) {
> +			test_get_offset(pkts_out, len,
> +			    fragment_offset, tests[i].ipv);
>  			test_free_fragments(pkts_out, len);
> +		}
> 
>  		printf("%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",
> +				    i, j, fragment_offset[j],
> +				    rte_cpu_to_be_16(
> +					tests[i].expected_fragment_offset[j]));
> +				RTE_TEST_ASSERT_EQUAL(fragment_offset[j],
> +				    rte_cpu_to_be_16(
> +					tests[i].expected_fragment_offset[j]),
> +				    "Failed case %zd.\n", i);
> +			}
> +		}
> +
>  	}
> 
>  	return result;
> --
> 1.8.3.1
> 

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

On Mon, Oct 25, 2021 at 9:59 AM huichao cai <chcchc88@163.com> wrote:
>
> Add the test content of the fragment_offset(offset and MF)
> to the test_ip_frag function. Add test data for a fragment
> that is not the last fragment.
>
> Signed-off-by: huichao cai <chcchc88@163.com>

The CI raises a regression on the ip frag test.

DPDK_TEST='ipfrag_autotest'
/home/runner/work/dpdk/dpdk/build/app/test/dpdk-test
--file-prefix=ipfrag_autotest
--- stdout ---
RTE>>ipfrag_autotest^M
 + ------------------------------------------------------- +
 + Test Suite : IP Frag Unit Test Suite
 + ------------------------------------------------------- +
0: checking 2 with 2
0-0: checking 32 with 32
0-1: checking 40192 with 40192
1: checking 2 with 2
1-0: checking 32 with 32
1-1: checking 40192 with 40192
2: checking 3 with 3
2-0: checking 32 with 32
2-1: checking 18464 with 18464
2-2: checking 36864 with 36864
3: checking -22 with -22
4: checking -95 with -95
5: checking 3 with 3
5-0: checking 32 with 32
5-1: checking 18464 with 18464
5-2: checking 36864 with 36864
6: checking 3 with 3
6-0: checking 6688 with 3360
 + TestCase [ 0] : test_ip_frag failed
 + ------------------------------------------------------- +
 + Test Suite Summary : IP Frag Unit Test Suite
 + ------------------------------------------------------- +
 + Tests Total :        1
 + Tests Skipped :      0
 + Tests Executed :     1
 + Tests Unsupported:   0
 + Tests Passed :       0
 + Tests Failed :       1
 + ------------------------------------------------------- +
Test Failed
RTE>>
--- stderr ---



Please investigate, thanks.
  

Hi Marchand


>6-0: checking 6688 with 3360
This test case failed because there was a bug in the "rte_ipv4_fragmentation.c" file.
It is this test case that discovers this bug. A patch to fix the bug has been received.
Please run ci again.Thanks.
The fix bug patche is:
ip_frag: fix the buf of fragmenting IPv4 fragment - Patchwork (dpdk.org)


Huichao Cai
  

Hello,

On Tue, Nov 9, 2021 at 3:22 AM Huichao Cai <chcchc88@163.com> wrote:
> >6-0: checking 6688 with 3360
>
> This test case failed because there was a bug in the "rte_ipv4_fragmentation.c" file.
> It is this test case that discovers this bug. A patch to fix the bug has been received.

Why was it separate from the fix?
I could not tell from this current patch that there was a dependency.
It could (should?) have been a single patch.


> The fix bug patche is:
> ip_frag: fix the buf of fragmenting IPv4 fragment - Patchwork (dpdk.org)

A link to patchwork would avoid me wasting time looking for it.
I guess this is the patch Thomas merged last night.


On the patch itself, the title is vague.
It should summarize what the change adds to the unit tests.
test/ipfrag: check fragment offsets

Thanks.
  

Hi Marchand


>It could (should?) have been a single patch.
--Yes, it can.I think the test unit is missing the offsets test content, so add this patch, and can also test the previous patch.
>A link to patchwork would avoid me wasting time looking for it.

>I guess this is the patch Thomas merged last night.
--I can see a link in my sent message，As in the screenshot below：
--But I'm not sure why you didn't get the link when you got the email, I'm sorry.



>On the patch itself, the title is vague.
>It should summarize what the change adds to the unit tests.

>test/ipfrag: check fragment offsets
--Thank you for reminding me, do I need to send another patch to modify the title?
Huichao Cai
  

Hi Marchand


I think it should be a matter of message text format, maybe I should send a link in text form, as follows：

https://patchwork.dpdk.org/project/dpdk/patch/1635148553-50086-1-git-send-email-chcchc88@163.com/


Thank you!


Huichao Cai
  

On Tue, Nov 9, 2021 at 10:03 AM Huichao Cai <chcchc88@163.com> wrote:
> >It could (should?) have been a single patch.
> --Yes, it can.I think the test unit is missing the offsets test content, so add this patch, and can also test the previous patch.

Too late, Thomas merged the fix in the library already.
Please consider this for future contributions.


> >A link to patchwork would avoid me wasting time looking for it.
>
> >I guess this is the patch Thomas merged last night.
> --I can see a link in my sent message，As in the screenshot below：
> --But I'm not sure why you didn't get the link when you got the email, I'm sorry.

I filter html.
I just saw your other reply: yes prefer plain text.

>
> >On the patch itself, the title is vague.
> >It should summarize what the change adds to the unit tests.
>
> >test/ipfrag: check fragment offsets
> --Thank you for reminding me, do I need to send another patch to modify the title?

You seem ok with this title, I'll go with it.
  

>You seem ok with this title, I'll go with it.
-Thank you!
  

On Thu, Oct 28, 2021 at 2:25 PM Ananyev, Konstantin
<konstantin.ananyev@intel.com> wrote:
> > Add the test content of the fragment_offset(offset and MF)
> > to the test_ip_frag function. Add test data for a fragment
> > that is not the last fragment.
> >
> > Signed-off-by: huichao cai <chcchc88@163.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied, thanks.