[PATCH v2 01/10] app/test: do not duplicated loop variable

[PATCH v2 01/10] app/test: do not duplicated loop variable

[Stephen Hemminger]

Do not use same variable for outer and inner loop in bonding test.
Since the loop is just freeing the resulting burst use bulk free.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 92073ef961ee ("bond: unit tests")
Cc: declan.doherty@intel.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_link_bonding.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index 4d54706c21..805613d7dd 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -2288,12 +2288,7 @@ test_activebackup_rx_burst(void)
 		}
 
 		/* free mbufs */
-		for (i = 0; i < MAX_PKT_BURST; i++) {
-			if (rx_pkt_burst[i] != NULL) {
-				rte_pktmbuf_free(rx_pkt_burst[i]);
-				rx_pkt_burst[i] = NULL;
-			}
-		}
+		rte_pktmbuf_free_bulk(rx_pkt_burst, burst_size);
 
 		/* reset bonding device stats */
 		rte_eth_stats_reset(test_params->bonding_port_id);
-- 
2.45.2

Re: [PATCH v2 01/10] app/test: do not duplicated loop variable

[Bruce Richardson]

On Thu, Nov 14, 2024 at 11:24:59AM -0800, Stephen Hemminger wrote:
> Do not use same variable for outer and inner loop in bonding test.
> Since the loop is just freeing the resulting burst use bulk free.
> 
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> 
> Fixes: 92073ef961ee ("bond: unit tests")
> Cc: declan.doherty@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v2 02/10] app/test: fix typo in address compare

[Stephen Hemminger]

The first argument of 'memcmp' function was equal to the second argument.
Therefore ASSERT would always be true.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 92073ef961ee ("bond: unit tests")
Cc: declan.doherty@intel.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_link_bonding.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_link_bonding.c b/app/test/test_link_bonding.c
index 805613d7dd..b752a5ecbf 100644
--- a/app/test/test_link_bonding.c
+++ b/app/test/test_link_bonding.c
@@ -792,7 +792,7 @@ test_set_primary_member(void)
 				&read_mac_addr),
 				"Failed to get mac address (port %d)",
 				test_params->bonding_port_id);
-		TEST_ASSERT_SUCCESS(memcmp(&read_mac_addr, &read_mac_addr,
+		TEST_ASSERT_SUCCESS(memcmp(expected_mac_addr, &read_mac_addr,
 				sizeof(read_mac_addr)),
 				"bonding port mac address not set to that of primary port\n");
 
-- 
2.45.2

[PATCH v2 03/10] app/test: fix paren typo

[Stephen Hemminger]

The parenthesis were in the wrong place so that comparison
took precedence over assignment in handling IPv6 extension
headers.  Break up the loop condition to avoid the problem.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 15ccc647526e ("test/security: test inline reassembly with multi-segment")
Cc: ndabilpuram@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_security_inline_proto_vectors.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/test/test_security_inline_proto_vectors.h b/app/test/test_security_inline_proto_vectors.h
index b3d724bac6..86dfa54777 100644
--- a/app/test/test_security_inline_proto_vectors.h
+++ b/app/test/test_security_inline_proto_vectors.h
@@ -519,10 +519,12 @@ test_vector_payload_populate(struct ip_reassembly_test_packet *pkt,
 			if (extra_data_sum) {
 				proto = hdr->proto;
 				p += sizeof(struct rte_ipv6_hdr);
-				while (proto != IPPROTO_FRAGMENT &&
-				       (proto = rte_ipv6_get_next_ext(p, proto, &ext_len) >= 0))
+				while (proto != IPPROTO_FRAGMENT) {
+					proto = rte_ipv6_get_next_ext(p, proto, &ext_len);
+					if (proto < 0)
+						break;
 					p += ext_len;
-
+				}
 				/* Found fragment header, update the frag offset */
 				if (proto == IPPROTO_FRAGMENT) {
 					frag_ext = (struct rte_ipv6_fragment_ext *)p;
-- 
2.45.2

Re: [PATCH v2 03/10] app/test: fix paren typo

[Bruce Richardson]

On Thu, Nov 14, 2024 at 11:25:01AM -0800, Stephen Hemminger wrote:
> The parenthesis were in the wrong place so that comparison
> took precedence over assignment in handling IPv6 extension
> headers.  Break up the loop condition to avoid the problem.
> 
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> 
> Fixes: 15ccc647526e ("test/security: test inline reassembly with multi-segment")
> Cc: ndabilpuram@marvell.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v2 05/10] app/test: fix TLS zero length record

[Stephen Hemminger]

The code was duplicating the same condition three times?
Reading the commit message, the intention was:

    Add unit tests to verify the zero len TLS records. Zero len packets are
    allowed when content type is app data while zero packet length with
    other content type (such as handshake) would result in an error.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 79a58624369a ("test/security: verify zero length TLS records")
Cc: vvelumuri@marvell.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_cryptodev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index c647baeee1..a33ef574cc 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -12253,10 +12253,7 @@ test_tls_record_proto_all(const struct tls_record_test_flags *flags)
 		if (flags->skip_sess_destroy && sec_session_outb == NULL)
 			sec_session_outb = ut_params->sec_session;
 
-		if (flags->zero_len &&
-		    ((flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE) ||
-		    (flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE) ||
-		    (flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE))) {
+		if (flags->zero_len && flags->content_type != TLS_RECORD_TEST_CONTENT_TYPE_APP) {
 			if (ret == TEST_SUCCESS)
 				return TEST_FAILED;
 			goto skip_decrypt;
-- 
2.45.2

[PATCH v2 06/10] app/test: fix operator precedence bug

[Stephen Hemminger]

The test loop was much shorter than desired because when
MAX_NUM is defined with out paren's the divide operator /
takes precedence over shift.

But when MAX_NUM is fixed, some tests take too long
and have to modified to avoid running over full N^2
space of 1<<20.

Note: this is a very old bug, goes back to 2013.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 1fb8b07ee511 ("app: add some tests")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_common.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 21eb2285e1..6dbd7fc9a9 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -9,11 +9,12 @@
 #include <rte_common.h>
 #include <rte_bitops.h>
 #include <rte_hexdump.h>
+#include <rte_random.h>
 #include <rte_pause.h>
 
 #include "test.h"
 
-#define MAX_NUM 1 << 20
+#define MAX_NUM (1 << 20)
 
 #define FAIL(x)\
 	{printf(x "() test failed!\n");\
@@ -218,19 +219,21 @@ test_align(void)
 		}
 	}
 
-	for (p = 1; p <= MAX_NUM / 2; p++) {
-		for (i = 1; i <= MAX_NUM / 2; i++) {
-			val = RTE_ALIGN_MUL_CEIL(i, p);
-			if (val % p != 0 || val < i)
-				FAIL_ALIGN("RTE_ALIGN_MUL_CEIL", i, p);
-			val = RTE_ALIGN_MUL_FLOOR(i, p);
-			if (val % p != 0 || val > i)
-				FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
-			val = RTE_ALIGN_MUL_NEAR(i, p);
-			if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
-				& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
-				FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
-		}
+	/* testing the whole space of 2^20^2 takes too long. */
+	for (j = 1; j <= MAX_NUM ; j++) {
+		i = rte_rand_max(MAX_NUM - 1) + 1;
+		p = rte_rand_max(MAX_NUM - 1) + 1;
+
+		val = RTE_ALIGN_MUL_CEIL(i, p);
+		if (val % p != 0 || val < i)
+			FAIL_ALIGN("RTE_ALIGN_MUL_CEIL", i, p);
+		val = RTE_ALIGN_MUL_FLOOR(i, p);
+		if (val % p != 0 || val > i)
+			FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
+		val = RTE_ALIGN_MUL_NEAR(i, p);
+		if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
+				     & (val != RTE_ALIGN_MUL_FLOOR(i, p))))
+			FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
 	}
 
 	return 0;
-- 
2.45.2

Re: [PATCH v2 06/10] app/test: fix operator precedence bug

[Bruce Richardson]

On Thu, Nov 14, 2024 at 11:25:04AM -0800, Stephen Hemminger wrote:
> The test loop was much shorter than desired because when
> MAX_NUM is defined with out paren's the divide operator /
> takes precedence over shift.
> 
> But when MAX_NUM is fixed, some tests take too long
> and have to modified to avoid running over full N^2
> space of 1<<20.
> 
> Note: this is a very old bug, goes back to 2013.
> 
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> 
> Fixes: 1fb8b07ee511 ("app: add some tests")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v2 07/10] test/eal: fix core check in c flag test

[Stephen Hemminger]

The expression for checking which lcore is enabled for 0-7
was wrong (missing case for 6).

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: b0209034f2bb ("test/eal: check number of cores before running subtests")
Cc: msantana@redhat.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_eal_flags.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
index d37d6b8627..e32f83d3c8 100644
--- a/app/test/test_eal_flags.c
+++ b/app/test/test_eal_flags.c
@@ -677,8 +677,8 @@ test_missing_c_flag(void)
 
 	if (rte_lcore_is_enabled(0) && rte_lcore_is_enabled(1) &&
 	    rte_lcore_is_enabled(2) && rte_lcore_is_enabled(3) &&
-	    rte_lcore_is_enabled(3) && rte_lcore_is_enabled(5) &&
-	    rte_lcore_is_enabled(4) && rte_lcore_is_enabled(7) &&
+	    rte_lcore_is_enabled(4) && rte_lcore_is_enabled(5) &&
+	    rte_lcore_is_enabled(6) && rte_lcore_is_enabled(7) &&
 	    launch_proc(argv29) != 0) {
 		printf("Error - "
 		       "process did not run ok with valid corelist value\n");
-- 
2.45.2

Re: [PATCH v2 07/10] test/eal: fix core check in c flag test

[Bruce Richardson]

On Thu, Nov 14, 2024 at 11:25:05AM -0800, Stephen Hemminger wrote:
> The expression for checking which lcore is enabled for 0-7
> was wrong (missing case for 6).
> 
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> 
> Fixes: b0209034f2bb ("test/eal: check number of cores before running subtests")
> Cc: msantana@redhat.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Just wondering would it not be better/safer to put in an actual loop check
here? 
However, I'm also ok with keeping the fix as-is, so:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>


> ---
>  app/test/test_eal_flags.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index d37d6b8627..e32f83d3c8 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -677,8 +677,8 @@ test_missing_c_flag(void)
>  
>  	if (rte_lcore_is_enabled(0) && rte_lcore_is_enabled(1) &&
>  	    rte_lcore_is_enabled(2) && rte_lcore_is_enabled(3) &&
> -	    rte_lcore_is_enabled(3) && rte_lcore_is_enabled(5) &&
> -	    rte_lcore_is_enabled(4) && rte_lcore_is_enabled(7) &&
> +	    rte_lcore_is_enabled(4) && rte_lcore_is_enabled(5) &&
> +	    rte_lcore_is_enabled(6) && rte_lcore_is_enabled(7) &&
>  	    launch_proc(argv29) != 0) {
>  		printf("Error - "
>  		       "process did not run ok with valid corelist value\n");
> -- 
> 2.45.2
> 

[PATCH v2 09/10] app/test-pmd: avoid potential outside of array reference

[Stephen Hemminger]

The order of comparison is wrong, and potentially allows
referencing past the array.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 3e3edab530a1 ("ethdev: add flow quota")
Cc: getelson@nvidia.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/cmdline_flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 1e4f2ebc55..9e4fc2d95d 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -12892,7 +12892,7 @@ comp_names_to_index(struct context *ctx, const struct token *token,
 	RTE_SET_USED(token);
 	if (!buf)
 		return names_size;
-	if (names[ent] && ent < names_size)
+	if (ent < names_size && names[ent] != NULL)
 		return rte_strscpy(buf, names[ent], size);
 	return -1;
 
-- 
2.45.2

Re: [PATCH v2 09/10] app/test-pmd: avoid potential outside of array reference

[Bruce Richardson]

On Thu, Nov 14, 2024 at 11:25:07AM -0800, Stephen Hemminger wrote:
> The order of comparison is wrong, and potentially allows
> referencing past the array.
> 
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> 
> Fixes: 3e3edab530a1 ("ethdev: add flow quota")
> Cc: getelson@nvidia.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address

[Stephen Hemminger]

There was useless loop when looking at the DMA address.
It looks like it was meant to skip whitespace before
calling strtok.

Good time to replace strtok with strtok_r as well.

Link: https://pvs-studio.com/en/blog/posts/cpp/1179/

Fixes: 623dc9364dc6 ("app/dma-perf: introduce DMA performance test")
Cc: cheng1.jiang@intel.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-dma-perf/main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
index 18219918cc..dccb0a3541 100644
--- a/app/test-dma-perf/main.c
+++ b/app/test-dma-perf/main.c
@@ -217,27 +217,27 @@ parse_lcore_dma(struct test_configure *test_case, const char *value)
 	struct lcore_dma_map_t *lcore_dma_map;
 	char *input, *addrs;
 	char *ptrs[2];
-	char *start, *end, *substr;
+	char *start, *end, *substr, *saveptr;
 	uint16_t lcore_id;
 	int ret = 0;
 
 	if (test_case == NULL || value == NULL)
 		return -1;
 
-	input = strndup(value, strlen(value) + 1);
+	input = strdup(value);
 	if (input == NULL)
 		return -1;
-	addrs = input;
 
-	while (*addrs == '\0')
-		addrs++;
+	addrs = input;
+	while (isspace(*addrs))
+		++addrs;
 	if (*addrs == '\0') {
 		fprintf(stderr, "No input DMA addresses\n");
 		ret = -1;
 		goto out;
 	}
 
-	substr = strtok(addrs, ",");
+	substr = strtok_r(addrs, ",", &saveptr);
 	if (substr == NULL) {
 		fprintf(stderr, "No input DMA address\n");
 		ret = -1;
-- 
2.45.2

Re: [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address

[Bruce Richardson]

On Thu, Nov 14, 2024 at 11:25:08AM -0800, Stephen Hemminger wrote:
> There was useless loop when looking at the DMA address.
> It looks like it was meant to skip whitespace before
> calling strtok.
> 
> Good time to replace strtok with strtok_r as well.
> 
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
> 
> Fixes: 623dc9364dc6 ("app/dma-perf: introduce DMA performance test")
> Cc: cheng1.jiang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

One comment inline below. With that fixed:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  app/test-dma-perf/main.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
> index 18219918cc..dccb0a3541 100644
> --- a/app/test-dma-perf/main.c
> +++ b/app/test-dma-perf/main.c
> @@ -217,27 +217,27 @@ parse_lcore_dma(struct test_configure *test_case, const char *value)
>  	struct lcore_dma_map_t *lcore_dma_map;
>  	char *input, *addrs;
>  	char *ptrs[2];
> -	char *start, *end, *substr;
> +	char *start, *end, *substr, *saveptr;
>  	uint16_t lcore_id;
>  	int ret = 0;
>  
>  	if (test_case == NULL || value == NULL)
>  		return -1;
>  
> -	input = strndup(value, strlen(value) + 1);
> +	input = strdup(value);
>  	if (input == NULL)
>  		return -1;
> -	addrs = input;
>  
> -	while (*addrs == '\0')
> -		addrs++;
> +	addrs = input;
> +	while (isspace(*addrs))
> +		++addrs;
>  	if (*addrs == '\0') {
>  		fprintf(stderr, "No input DMA addresses\n");
>  		ret = -1;
>  		goto out;
>  	}
>  
> -	substr = strtok(addrs, ",");
> +	substr = strtok_r(addrs, ",", &saveptr);

Don't need to use strtok here at all. Just use strchr, and then no need for
a new temporary variable.

>  	if (substr == NULL) {
>  		fprintf(stderr, "No input DMA address\n");
>  		ret = -1;
> -- 
> 2.45.2
>