* [RFC 01/10] app/test: do not duplicate loop variable
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 02/10] app/test: fix typo in mac address compare Stephen Hemminger
` (11 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, Chas Williams, Min Hu (Connor),
Pablo de Lara
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
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 02/10] app/test: fix typo in mac address compare
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
2024-11-14 0:12 ` [RFC 01/10] app/test: do not duplicate loop variable Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 03/10] app/test: fix paren typo Stephen Hemminger
` (10 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, Chas Williams, Min Hu (Connor),
Pablo de Lara
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
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 03/10] app/test: fix paren typo
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
2024-11-14 0:12 ` [RFC 01/10] app/test: do not duplicate loop variable Stephen Hemminger
2024-11-14 0:12 ` [RFC 02/10] app/test: fix typo in mac address compare Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 04/10] app/test: avoid duplicate initialization Stephen Hemminger
` (9 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, ndabilpuram, Akhil Goyal, Anoob Joseph
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
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 04/10] app/test: avoid duplicate initialization
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (2 preceding siblings ...)
2024-11-14 0:12 ` [RFC 03/10] app/test: fix paren typo Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 05/10] app/test: fix TLS zero length record Stephen Hemminger
` (8 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, jerin.jacob, Abhinandan Gujjar, Bruce Richardson
The event_dev_config initialization had duplicate assignments
to the same element. Change to use structure initialization
so that compiler will catch this type of bug.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
Cc: jerin.jacob@caviumnetworks.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
app/test/test_event_crypto_adapter.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 9d38a66bfa..ab24e30a97 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -1154,21 +1154,17 @@ configure_cryptodev(void)
static inline void
evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
- struct rte_event_dev_info *info)
+ const struct rte_event_dev_info *info)
{
- memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
- dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
- dev_conf->nb_event_ports = NB_TEST_PORTS;
- dev_conf->nb_event_queues = NB_TEST_QUEUES;
- dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
- dev_conf->nb_event_port_dequeue_depth =
- info->max_event_port_dequeue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_events_limit =
- info->max_num_events;
+ *dev_conf = (struct rte_event_dev_config) {
+ .dequeue_timeout_ns = info->min_dequeue_timeout_ns,
+ .nb_event_ports = NB_TEST_PORTS,
+ .nb_event_queues = NB_TEST_QUEUES,
+ .nb_event_queue_flows = info->max_event_queue_flows,
+ .nb_event_port_dequeue_depth = info->max_event_port_dequeue_depth,
+ .nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth,
+ .nb_events_limit = info->max_num_events,
+ };
}
static int
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 05/10] app/test: fix TLS zero length record
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (3 preceding siblings ...)
2024-11-14 0:12 ` [RFC 04/10] app/test: avoid duplicate initialization Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 06/10] test/eal: fix core check in c flag test Stephen Hemminger
` (7 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, vvelumuri, Akhil Goyal, Fan Zhang, Anoob Joseph
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
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 06/10] test/eal: fix core check in c flag test
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (4 preceding siblings ...)
2024-11-14 0:12 ` [RFC 05/10] app/test: fix TLS zero length record Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 07/10] app/test-pmd: remove redundant condition Stephen Hemminger
` (6 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, msantana, Tyler Retzlaff, Michael Santana,
Aaron Conole, David Marchand
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
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 07/10] app/test-pmd: remove redundant condition
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (5 preceding siblings ...)
2024-11-14 0:12 ` [RFC 06/10] test/eal: fix core check in c flag test Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 08/10] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
` (5 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, haifeil, Aman Singh, Matan Azrad, Ajit Khaparde
The loop over policy actions will always exit when it sees
the flow end action, so the next check is redundant.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
Cc: haifeil@nvidia.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
app/test-pmd/config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..32c4e86c84 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
- if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
+ if (act_n > 0)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
@@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
printf(" %s\n", buf);
}
}
-
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 08/10] app/test-pmd: avoid potential outside of array reference
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (6 preceding siblings ...)
2024-11-14 0:12 ` [RFC 07/10] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 0:12 ` [RFC 09/10] app/test-dma-perf: fix parsing of dma address Stephen Hemminger
` (4 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, getelson, Ori Kam, Aman Singh
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
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [RFC 09/10] app/test-dma-perf: fix parsing of dma address
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (7 preceding siblings ...)
2024-11-14 0:12 ` [RFC 08/10] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 7:00 ` Morten Brørup
2024-11-15 7:19 ` fengchengwen
2024-11-14 0:12 ` [RFC 10/10] app/test: fix operator precedence bug Stephen Hemminger
` (3 subsequent siblings)
12 siblings, 2 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, cheng1.jiang, Cheng Jiang, Chengwen Feng,
Jiayu Hu, Morten Brørup, Anoob Joseph, Chenbo Xia
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
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
app/test-dma-perf/main.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
index 18219918cc..dabf4e02e6 100644
--- a/app/test-dma-perf/main.c
+++ b/app/test-dma-perf/main.c
@@ -217,19 +217,18 @@ 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')
+ while (*addrs == '\0' && isspace(*addrs))
addrs++;
if (*addrs == '\0') {
fprintf(stderr, "No input DMA addresses\n");
@@ -237,7 +236,7 @@ parse_lcore_dma(struct test_configure *test_case, const char *value)
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
^ permalink raw reply [flat|nested] 69+ messages in thread* RE: [RFC 09/10] app/test-dma-perf: fix parsing of dma address
2024-11-14 0:12 ` [RFC 09/10] app/test-dma-perf: fix parsing of dma address Stephen Hemminger
@ 2024-11-14 7:00 ` Morten Brørup
2024-11-14 16:21 ` Stephen Hemminger
2024-11-15 7:19 ` fengchengwen
1 sibling, 1 reply; 69+ messages in thread
From: Morten Brørup @ 2024-11-14 7:00 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: cheng1.jiang, Cheng Jiang, Chengwen Feng, Jiayu Hu, Anoob Joseph,
Chenbo Xia
> + while (*addrs == '\0' && isspace(*addrs))
> addrs++;
This is never going to happen. Did you mean ||?
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [RFC 09/10] app/test-dma-perf: fix parsing of dma address
2024-11-14 7:00 ` Morten Brørup
@ 2024-11-14 16:21 ` Stephen Hemminger
0 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 16:21 UTC (permalink / raw)
To: Morten Brørup
Cc: dev, cheng1.jiang, Cheng Jiang, Chengwen Feng, Jiayu Hu,
Anoob Joseph, Chenbo Xia
On Thu, 14 Nov 2024 08:00:52 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> > + while (*addrs == '\0' && isspace(*addrs))
> > addrs++;
>
> This is never going to happen. Did you mean ||?
Good catch.
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [RFC 09/10] app/test-dma-perf: fix parsing of dma address
2024-11-14 0:12 ` [RFC 09/10] app/test-dma-perf: fix parsing of dma address Stephen Hemminger
2024-11-14 7:00 ` Morten Brørup
@ 2024-11-15 7:19 ` fengchengwen
1 sibling, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-15 7:19 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: cheng1.jiang, Cheng Jiang, Jiayu Hu, Morten Brørup,
Anoob Joseph, Chenbo Xia
On 2024/11/14 8:12, 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.
Incomplete modification for strtok, I suggest the strtok adopt HaiJie's patchset [1],
This commit just modify non-strtok logic.
[1] https://inbox.dpdk.org/dev/20241108110404.18317-6-haijie1@huawei.com/
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: 623dc9364dc6 ("app/dma-perf: introduce DMA performance test")
> Cc: cheng1.jiang@intel.com
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> app/test-dma-perf/main.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
> index 18219918cc..dabf4e02e6 100644
> --- a/app/test-dma-perf/main.c
> +++ b/app/test-dma-perf/main.c
> @@ -217,19 +217,18 @@ 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')
> + while (*addrs == '\0' && isspace(*addrs))
> addrs++;
> if (*addrs == '\0') {
> fprintf(stderr, "No input DMA addresses\n");
> @@ -237,7 +236,7 @@ parse_lcore_dma(struct test_configure *test_case, const char *value)
> goto out;
> }
>
> - substr = strtok(addrs, ",");
> + substr = strtok_r(addrs, ",", &saveptr);
> if (substr == NULL) {
> fprintf(stderr, "No input DMA address\n");
> ret = -1;
^ permalink raw reply [flat|nested] 69+ messages in thread
* [RFC 10/10] app/test: fix operator precedence bug
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (8 preceding siblings ...)
2024-11-14 0:12 ` [RFC 09/10] app/test-dma-perf: fix parsing of dma address Stephen Hemminger
@ 2024-11-14 0:12 ` Stephen Hemminger
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (2 subsequent siblings)
12 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 0:12 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, Tyler Retzlaff
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")
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 00/10] Bug fixes for unit tests
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (9 preceding siblings ...)
2024-11-14 0:12 ` [RFC 10/10] app/test: fix operator precedence bug Stephen Hemminger
@ 2024-11-14 19:24 ` Stephen Hemminger
2024-11-14 19:24 ` [PATCH v2 01/10] app/test: do not duplicated loop variable Stephen Hemminger
` (9 more replies)
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
12 siblings, 10 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:24 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
Recent blog post from PVS studio referenced lots bugs found by
their analyzer against DPDK. This set addresses the ones in
the test suite.
v2 - fix dma address parsing
- add stable where needed
Stephen Hemminger (10):
app/test: do not duplicated loop variable
app/test: fix typo in address compare
app/test: fix paren typo
app/test: avoid duplicate initialization
app/test: fix TLS zero length record
app/test: fix operator precedence bug
test/eal: fix core check in c flag test
app/test-pmd: remove redundant condition
app/test-pmd: avoid potential outside of array reference
app/test-dma-perf: fix parsing of DMA address
app/test-dma-perf/main.c | 12 +++----
app/test-pmd/cmdline_flow.c | 2 +-
app/test-pmd/config.c | 3 +-
app/test/test_common.c | 31 ++++++++++---------
app/test/test_cryptodev.c | 5 +--
app/test/test_eal_flags.c | 4 +--
app/test/test_event_crypto_adapter.c | 24 ++++++--------
app/test/test_link_bonding.c | 9 ++----
app/test/test_security_inline_proto_vectors.h | 8 +++--
9 files changed, 45 insertions(+), 53 deletions(-)
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 01/10] app/test: do not duplicated loop variable
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
@ 2024-11-14 19:24 ` Stephen Hemminger
2024-11-15 9:01 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 02/10] app/test: fix typo in address compare Stephen Hemminger
` (8 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:24 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, stable, Chas Williams,
Min Hu (Connor),
Pablo de Lara
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 02/10] app/test: fix typo in address compare
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
2024-11-14 19:24 ` [PATCH v2 01/10] app/test: do not duplicated loop variable Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-14 19:25 ` [PATCH v2 03/10] app/test: fix paren typo Stephen Hemminger
` (7 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, stable, Chas Williams,
Min Hu (Connor),
Pablo de Lara
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 03/10] app/test: fix paren typo
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
2024-11-14 19:24 ` [PATCH v2 01/10] app/test: do not duplicated loop variable Stephen Hemminger
2024-11-14 19:25 ` [PATCH v2 02/10] app/test: fix typo in address compare Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-15 9:02 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 04/10] app/test: avoid duplicate initialization Stephen Hemminger
` (6 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, ndabilpuram, stable, Akhil Goyal, Anoob Joseph
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 03/10] app/test: fix paren typo
2024-11-14 19:25 ` [PATCH v2 03/10] app/test: fix paren typo Stephen Hemminger
@ 2024-11-15 9:02 ` Bruce Richardson
0 siblings, 0 replies; 69+ messages in thread
From: Bruce Richardson @ 2024-11-15 9:02 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, ndabilpuram, stable, Akhil Goyal, Anoob Joseph
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>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 04/10] app/test: avoid duplicate initialization
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (2 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 03/10] app/test: fix paren typo Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-15 9:03 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 05/10] app/test: fix TLS zero length record Stephen Hemminger
` (5 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, jerin.jacob, Abhinandan Gujjar, Bruce Richardson
The event_dev_config initialization had duplicate assignments
to the same element. Change to use structure initialization
so that compiler will catch this type of bug.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
Cc: jerin.jacob@caviumnetworks.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
app/test/test_event_crypto_adapter.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 9d38a66bfa..ab24e30a97 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -1154,21 +1154,17 @@ configure_cryptodev(void)
static inline void
evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
- struct rte_event_dev_info *info)
+ const struct rte_event_dev_info *info)
{
- memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
- dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
- dev_conf->nb_event_ports = NB_TEST_PORTS;
- dev_conf->nb_event_queues = NB_TEST_QUEUES;
- dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
- dev_conf->nb_event_port_dequeue_depth =
- info->max_event_port_dequeue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_events_limit =
- info->max_num_events;
+ *dev_conf = (struct rte_event_dev_config) {
+ .dequeue_timeout_ns = info->min_dequeue_timeout_ns,
+ .nb_event_ports = NB_TEST_PORTS,
+ .nb_event_queues = NB_TEST_QUEUES,
+ .nb_event_queue_flows = info->max_event_queue_flows,
+ .nb_event_port_dequeue_depth = info->max_event_port_dequeue_depth,
+ .nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth,
+ .nb_events_limit = info->max_num_events,
+ };
}
static int
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 05/10] app/test: fix TLS zero length record
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (3 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 04/10] app/test: avoid duplicate initialization Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-14 19:25 ` [PATCH v2 06/10] app/test: fix operator precedence bug Stephen Hemminger
` (4 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, vvelumuri, stable, Akhil Goyal, Fan Zhang,
Anoob Joseph
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 06/10] app/test: fix operator precedence bug
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (4 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 05/10] app/test: fix TLS zero length record Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-15 9:06 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 07/10] test/eal: fix core check in c flag test Stephen Hemminger
` (3 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, stable, Tyler Retzlaff
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 06/10] app/test: fix operator precedence bug
2024-11-14 19:25 ` [PATCH v2 06/10] app/test: fix operator precedence bug Stephen Hemminger
@ 2024-11-15 9:06 ` Bruce Richardson
0 siblings, 0 replies; 69+ messages in thread
From: Bruce Richardson @ 2024-11-15 9:06 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, stable, Tyler Retzlaff
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>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 07/10] test/eal: fix core check in c flag test
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (5 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 06/10] app/test: fix operator precedence bug Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-15 9:07 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
` (2 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, msantana, stable, Tyler Retzlaff,
Michael Santana, David Marchand, Aaron Conole
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 07/10] test/eal: fix core check in c flag test
2024-11-14 19:25 ` [PATCH v2 07/10] test/eal: fix core check in c flag test Stephen Hemminger
@ 2024-11-15 9:07 ` Bruce Richardson
2024-11-15 19:53 ` Stephen Hemminger
0 siblings, 1 reply; 69+ messages in thread
From: Bruce Richardson @ 2024-11-15 9:07 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, msantana, stable, Tyler Retzlaff, Michael Santana,
David Marchand, Aaron Conole
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
>
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 07/10] test/eal: fix core check in c flag test
2024-11-15 9:07 ` Bruce Richardson
@ 2024-11-15 19:53 ` Stephen Hemminger
0 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 19:53 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, msantana, stable, Tyler Retzlaff, Michael Santana,
David Marchand, Aaron Conole
On Fri, 15 Nov 2024 09:07:42 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:
> 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>
My goal was to do minimum changes for now, to avoid introducing new bugs.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 08/10] app/test-pmd: remove redundant condition
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (6 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 07/10] test/eal: fix core check in c flag test Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-14 19:27 ` Ajit Khaparde
2024-11-15 9:08 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 09/10] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
2024-11-14 19:25 ` [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address Stephen Hemminger
9 siblings, 2 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, haifeil, Aman Singh, Ajit Khaparde, Matan Azrad
The loop over policy actions will always exit when it sees
the flow end action, so the next check is redundant.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
Cc: haifeil@nvidia.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
app/test-pmd/config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..32c4e86c84 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
- if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
+ if (act_n > 0)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
@@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
printf(" %s\n", buf);
}
}
-
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 08/10] app/test-pmd: remove redundant condition
2024-11-14 19:25 ` [PATCH v2 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-14 19:27 ` Ajit Khaparde
2024-11-15 9:08 ` Bruce Richardson
1 sibling, 0 replies; 69+ messages in thread
From: Ajit Khaparde @ 2024-11-14 19:27 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, haifeil, Aman Singh, Matan Azrad
[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]
On Thu, Nov 14, 2024 at 11:26 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The loop over policy actions will always exit when it sees
> the flow end action, so the next check is redundant.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
> Cc: haifeil@nvidia.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> app/test-pmd/config.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 88770b4dfc..32c4e86c84 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
> for (act_n = 0, start = act;
> act->type != RTE_FLOW_ACTION_TYPE_END; act++)
> act_n++;
> - if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
> + if (act_n > 0)
> policy.actions[i] = start;
> else
> policy.actions[i] = NULL;
> @@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
> printf(" %s\n", buf);
> }
> }
> -
> --
> 2.45.2
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 08/10] app/test-pmd: remove redundant condition
2024-11-14 19:25 ` [PATCH v2 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
2024-11-14 19:27 ` Ajit Khaparde
@ 2024-11-15 9:08 ` Bruce Richardson
1 sibling, 0 replies; 69+ messages in thread
From: Bruce Richardson @ 2024-11-15 9:08 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, haifeil, Aman Singh, Ajit Khaparde, Matan Azrad
On Thu, Nov 14, 2024 at 11:25:06AM -0800, Stephen Hemminger wrote:
> The loop over policy actions will always exit when it sees
> the flow end action, so the next check is redundant.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
> Cc: haifeil@nvidia.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 09/10] app/test-pmd: avoid potential outside of array reference
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (7 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-15 9:09 ` Bruce Richardson
2024-11-14 19:25 ` [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address Stephen Hemminger
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, getelson, stable, Ori Kam, Aman Singh
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
` (8 preceding siblings ...)
2024-11-14 19:25 ` [PATCH v2 09/10] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
@ 2024-11-14 19:25 ` Stephen Hemminger
2024-11-15 9:13 ` Bruce Richardson
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-14 19:25 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, cheng1.jiang, stable, Cheng Jiang,
Chengwen Feng, Yuan Wang, Morten Brørup, Jiayu Hu
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address
2024-11-14 19:25 ` [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address Stephen Hemminger
@ 2024-11-15 9:13 ` Bruce Richardson
2024-11-15 19:58 ` Stephen Hemminger
0 siblings, 1 reply; 69+ messages in thread
From: Bruce Richardson @ 2024-11-15 9:13 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, cheng1.jiang, stable, Cheng Jiang, Chengwen Feng, Yuan Wang,
Morten Brørup, Jiayu Hu
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
>
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v2 10/10] app/test-dma-perf: fix parsing of DMA address
2024-11-15 9:13 ` Bruce Richardson
@ 2024-11-15 19:58 ` Stephen Hemminger
0 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 19:58 UTC (permalink / raw)
To: Bruce Richardson
Cc: dev, cheng1.jiang, stable, Cheng Jiang, Chengwen Feng, Yuan Wang,
Morten Brørup, Jiayu Hu
On Fri, 15 Nov 2024 09:13:12 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:
> 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.
The issue is that the test is passing each comma seperated section to rte_strsplit
and that needs the first part to be null terminated.
But using strtok_r in only one spot is wrong, will fix.
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 00/10] bug fixes for unit tests
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (10 preceding siblings ...)
2024-11-14 19:24 ` [PATCH v2 00/10] Bug fixes for unit tests Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 01/10] app/test: do not duplicate loop variable Stephen Hemminger
` (9 more replies)
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
12 siblings, 10 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
Recent blog post from PVS studio referenced lots bugs found by
their analyzer against DPDK. This set addresses the ones in
the test suite.
v3 - minimize change to test-dma-perf
Stephen Hemminger (10):
app/test: do not duplicate loop variable
app/test: fix typo in address compare
app/test: fix paren typo
app/test: avoid duplicate initialization
app/test: fix TLS zero length record
app/test: fix operator precedence bug
test/eal: fix core check in c flag test
app/test-pmd: remove redundant condition
app/test-pmd: avoid potential outside of array reference
app/test-dma-perf: fix parsing of DMA address
app/test-dma-perf/main.c | 4 +--
app/test-pmd/cmdline_flow.c | 2 +-
app/test-pmd/config.c | 3 +-
app/test/test_common.c | 31 ++++++++++---------
app/test/test_cryptodev.c | 5 +--
app/test/test_eal_flags.c | 4 +--
app/test/test_event_crypto_adapter.c | 24 ++++++--------
app/test/test_link_bonding.c | 9 ++----
app/test/test_security_inline_proto_vectors.h | 8 +++--
9 files changed, 41 insertions(+), 49 deletions(-)
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 01/10] app/test: do not duplicate loop variable
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 02/10] app/test: fix typo in address compare Stephen Hemminger
` (8 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, stable, Bruce Richardson,
Chas Williams, Min Hu (Connor),
Pablo de Lara
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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 02/10] app/test: fix typo in address compare
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 01/10] app/test: do not duplicate loop variable Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 03/10] app/test: fix paren typo Stephen Hemminger
` (7 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, stable, Chas Williams,
Min Hu (Connor),
Pablo de Lara
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 03/10] app/test: fix paren typo
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 01/10] app/test: do not duplicate loop variable Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 02/10] app/test: fix typo in address compare Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 04/10] app/test: avoid duplicate initialization Stephen Hemminger
` (6 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, ndabilpuram, stable, Bruce Richardson,
Akhil Goyal, Anoob Joseph
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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 04/10] app/test: avoid duplicate initialization
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (2 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 03/10] app/test: fix paren typo Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-18 4:49 ` Gujjar, Abhinandan S
2024-11-15 20:06 ` [PATCH v3 05/10] app/test: fix TLS zero length record Stephen Hemminger
` (5 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, jerin.jacob, Bruce Richardson, Abhinandan Gujjar
The event_dev_config initialization had duplicate assignments
to the same element. Change to use structure initialization
so that compiler will catch this type of bug.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
Cc: jerin.jacob@caviumnetworks.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
app/test/test_event_crypto_adapter.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 9d38a66bfa..ab24e30a97 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -1154,21 +1154,17 @@ configure_cryptodev(void)
static inline void
evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
- struct rte_event_dev_info *info)
+ const struct rte_event_dev_info *info)
{
- memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
- dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
- dev_conf->nb_event_ports = NB_TEST_PORTS;
- dev_conf->nb_event_queues = NB_TEST_QUEUES;
- dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
- dev_conf->nb_event_port_dequeue_depth =
- info->max_event_port_dequeue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_events_limit =
- info->max_num_events;
+ *dev_conf = (struct rte_event_dev_config) {
+ .dequeue_timeout_ns = info->min_dequeue_timeout_ns,
+ .nb_event_ports = NB_TEST_PORTS,
+ .nb_event_queues = NB_TEST_QUEUES,
+ .nb_event_queue_flows = info->max_event_queue_flows,
+ .nb_event_port_dequeue_depth = info->max_event_port_dequeue_depth,
+ .nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth,
+ .nb_events_limit = info->max_num_events,
+ };
}
static int
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* RE: [PATCH v3 04/10] app/test: avoid duplicate initialization
2024-11-15 20:06 ` [PATCH v3 04/10] app/test: avoid duplicate initialization Stephen Hemminger
@ 2024-11-18 4:49 ` Gujjar, Abhinandan S
0 siblings, 0 replies; 69+ messages in thread
From: Gujjar, Abhinandan S @ 2024-11-18 4:49 UTC (permalink / raw)
To: Stephen Hemminger, dev; +Cc: jerin.jacob, Richardson, Bruce
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Saturday, November 16, 2024 1:37 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>;
> jerin.jacob@caviumnetworks.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Gujjar, Abhinandan S
> <abhinandan.gujjar@intel.com>
> Subject: [PATCH v3 04/10] app/test: avoid duplicate initialization
>
> The event_dev_config initialization had duplicate assignments to the same
> element. Change to use structure initialization so that compiler will catch this
> type of bug.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
> Cc: jerin.jacob@caviumnetworks.com
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> app/test/test_event_crypto_adapter.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/app/test/test_event_crypto_adapter.c
> b/app/test/test_event_crypto_adapter.c
> index 9d38a66bfa..ab24e30a97 100644
> --- a/app/test/test_event_crypto_adapter.c
> +++ b/app/test/test_event_crypto_adapter.c
> @@ -1154,21 +1154,17 @@ configure_cryptodev(void)
>
> static inline void
> evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
> - struct rte_event_dev_info *info)
> + const struct rte_event_dev_info *info)
> {
> - memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
> - dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
> - dev_conf->nb_event_ports = NB_TEST_PORTS;
> - dev_conf->nb_event_queues = NB_TEST_QUEUES;
> - dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
> - dev_conf->nb_event_port_dequeue_depth =
> - info->max_event_port_dequeue_depth;
> - dev_conf->nb_event_port_enqueue_depth =
> - info->max_event_port_enqueue_depth;
> - dev_conf->nb_event_port_enqueue_depth =
> - info->max_event_port_enqueue_depth;
> - dev_conf->nb_events_limit =
> - info->max_num_events;
> + *dev_conf = (struct rte_event_dev_config) {
> + .dequeue_timeout_ns = info->min_dequeue_timeout_ns,
> + .nb_event_ports = NB_TEST_PORTS,
> + .nb_event_queues = NB_TEST_QUEUES,
> + .nb_event_queue_flows = info->max_event_queue_flows,
> + .nb_event_port_dequeue_depth = info-
> >max_event_port_dequeue_depth,
> + .nb_event_port_enqueue_depth = info-
> >max_event_port_enqueue_depth,
> + .nb_events_limit = info->max_num_events,
> + };
> }
>
> static int
> --
> 2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 05/10] app/test: fix TLS zero length record
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (3 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 04/10] app/test: avoid duplicate initialization Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 06/10] app/test: fix operator precedence bug Stephen Hemminger
` (4 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, vvelumuri, stable, Akhil Goyal, Fan Zhang,
Anoob Joseph
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 06/10] app/test: fix operator precedence bug
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (4 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 05/10] app/test: fix TLS zero length record Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 07/10] test/eal: fix core check in c flag test Stephen Hemminger
` (3 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, stable, Bruce Richardson, Tyler Retzlaff
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 be 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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 07/10] test/eal: fix core check in c flag test
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (5 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 06/10] app/test: fix operator precedence bug Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
` (2 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, msantana, stable, Bruce Richardson,
Tyler Retzlaff, David Marchand, Michael Santana, Aaron Conole
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>
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 08/10] app/test-pmd: remove redundant condition
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (6 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 07/10] test/eal: fix core check in c flag test Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:52 ` Ajit Khaparde
2024-11-15 20:06 ` [PATCH v3 09/10] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 10/10] app/test-dma-perf: fix parsing of DMA address Stephen Hemminger
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, haifeil, Bruce Richardson, Aman Singh,
Matan Azrad, Ajit Khaparde
The loop over policy actions will always exit when it sees
the flow end action, so the next check is redundant.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
Cc: haifeil@nvidia.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
app/test-pmd/config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 88770b4dfc..32c4e86c84 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
- if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
+ if (act_n > 0)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
@@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
printf(" %s\n", buf);
}
}
-
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v3 08/10] app/test-pmd: remove redundant condition
2024-11-15 20:06 ` [PATCH v3 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-15 20:52 ` Ajit Khaparde
0 siblings, 0 replies; 69+ messages in thread
From: Ajit Khaparde @ 2024-11-15 20:52 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, haifeil, Bruce Richardson, Aman Singh, Matan Azrad
[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]
On Fri, Nov 15, 2024 at 12:08 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The loop over policy actions will always exit when it sees
> the flow end action, so the next check is redundant.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
> Cc: haifeil@nvidia.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> app/test-pmd/config.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 88770b4dfc..32c4e86c84 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
> for (act_n = 0, start = act;
> act->type != RTE_FLOW_ACTION_TYPE_END; act++)
> act_n++;
> - if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
> + if (act_n > 0)
> policy.actions[i] = start;
> else
> policy.actions[i] = NULL;
> @@ -7316,4 +7316,3 @@ show_mcast_macs(portid_t port_id)
> printf(" %s\n", buf);
> }
> }
> -
> --
> 2.45.2
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 09/10] app/test-pmd: avoid potential outside of array reference
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (7 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 08/10] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-15 20:06 ` [PATCH v3 10/10] app/test-dma-perf: fix parsing of DMA address Stephen Hemminger
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, getelson, stable, Bruce Richardson, Ori Kam,
Aman Singh
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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v3 10/10] app/test-dma-perf: fix parsing of DMA address
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
` (8 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 09/10] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
@ 2024-11-15 20:06 ` Stephen Hemminger
2024-11-18 2:44 ` fengchengwen
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-15 20:06 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, cheng1.jiang, stable, Bruce Richardson,
Cheng Jiang, Chengwen Feng, Yuan Wang, Chenbo Xia, Anoob Joseph,
Jiayu Hu
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>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
app/test-dma-perf/main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
index 18219918cc..8d46b1258b 100644
--- a/app/test-dma-perf/main.c
+++ b/app/test-dma-perf/main.c
@@ -229,8 +229,8 @@ parse_lcore_dma(struct test_configure *test_case, const char *value)
return -1;
addrs = input;
- while (*addrs == '\0')
- addrs++;
+ while (isspace(*addrs))
+ ++addrs;
if (*addrs == '\0') {
fprintf(stderr, "No input DMA addresses\n");
ret = -1;
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v3 10/10] app/test-dma-perf: fix parsing of DMA address
2024-11-15 20:06 ` [PATCH v3 10/10] app/test-dma-perf: fix parsing of DMA address Stephen Hemminger
@ 2024-11-18 2:44 ` fengchengwen
0 siblings, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-18 2:44 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: cheng1.jiang, stable, Bruce Richardson, Cheng Jiang, Yuan Wang,
Chenbo Xia, Anoob Joseph, Jiayu Hu
On 2024/11/16 4:06, 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.
Please delete this line, with this fixed:
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>
> 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>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> app/test-dma-perf/main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
> index 18219918cc..8d46b1258b 100644
> --- a/app/test-dma-perf/main.c
> +++ b/app/test-dma-perf/main.c
> @@ -229,8 +229,8 @@ parse_lcore_dma(struct test_configure *test_case, const char *value)
> return -1;
> addrs = input;
>
> - while (*addrs == '\0')
> - addrs++;
> + while (isspace(*addrs))
> + ++addrs;
> if (*addrs == '\0') {
> fprintf(stderr, "No input DMA addresses\n");
> ret = -1;
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 0/9] Bug fixes for standalone tests
2024-11-14 0:12 [RFC 00/10] unit test bugs Stephen Hemminger
` (11 preceding siblings ...)
2024-11-15 20:06 ` [PATCH v3 00/10] bug fixes for unit tests Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 1/9] app/test: do not duplicate loop variable Stephen Hemminger
` (9 more replies)
12 siblings, 10 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger
Recent blog post from PVS studio referenced lots bugs found by
their analyzer against DPDK. This set addresses the ones in
the test suite.
v4 - rebase and drop code that was already fixed (removed)
Stephen Hemminger (9):
app/test: do not duplicate loop variable
app/test: fix typo in address compare
app/test: fix paren typo
app/test: avoid duplicate initialization
app/test: fix TLS zero length record
app/test: fix operator precedence bug
test/eal: fix core check in c flag test
app/test-pmd: remove redundant condition
app/test-pmd: avoid potential outside of array reference
app/test-pmd/cmdline_flow.c | 2 +-
app/test-pmd/config.c | 3 +-
app/test/test_common.c | 31 ++++++++++---------
app/test/test_cryptodev.c | 5 +--
app/test/test_eal_flags.c | 4 +--
app/test/test_event_crypto_adapter.c | 24 ++++++--------
app/test/test_link_bonding.c | 9 ++----
app/test/test_security_inline_proto_vectors.h | 8 +++--
8 files changed, 39 insertions(+), 47 deletions(-)
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v4 1/9] app/test: do not duplicate loop variable
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-22 0:39 ` fengchengwen
2024-11-21 18:23 ` [PATCH v4 2/9] app/test: fix typo in address compare Stephen Hemminger
` (8 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, stable, Bruce Richardson,
Chas Williams, Min Hu (Connor),
Pablo de Lara
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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 1/9] app/test: do not duplicate loop variable
2024-11-21 18:23 ` [PATCH v4 1/9] app/test: do not duplicate loop variable Stephen Hemminger
@ 2024-11-22 0:39 ` fengchengwen
0 siblings, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-22 0:39 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: declan.doherty, stable, Bruce Richardson, Chas Williams,
Min Hu (Connor),
Pablo de Lara
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
On 2024/11/22 2:23, 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>
> ---
> 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);
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 2/9] app/test: fix typo in address compare
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 1/9] app/test: do not duplicate loop variable Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-22 0:37 ` fengchengwen
2024-11-22 8:54 ` Bruce Richardson
2024-11-21 18:23 ` [PATCH v4 3/9] app/test: fix paren typo Stephen Hemminger
` (7 subsequent siblings)
9 siblings, 2 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, declan.doherty, stable, Chas Williams,
Min Hu (Connor),
Pablo de Lara
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 2/9] app/test: fix typo in address compare
2024-11-21 18:23 ` [PATCH v4 2/9] app/test: fix typo in address compare Stephen Hemminger
@ 2024-11-22 0:37 ` fengchengwen
2024-11-22 8:54 ` Bruce Richardson
1 sibling, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-22 0:37 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: declan.doherty, stable, Chas Williams, Min Hu (Connor), Pablo de Lara
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
On 2024/11/22 2:23, Stephen Hemminger wrote:
> 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");
>
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 2/9] app/test: fix typo in address compare
2024-11-21 18:23 ` [PATCH v4 2/9] app/test: fix typo in address compare Stephen Hemminger
2024-11-22 0:37 ` fengchengwen
@ 2024-11-22 8:54 ` Bruce Richardson
1 sibling, 0 replies; 69+ messages in thread
From: Bruce Richardson @ 2024-11-22 8:54 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, declan.doherty, stable, Chas Williams, Min Hu (Connor),
Pablo de Lara
On Thu, Nov 21, 2024 at 10:23:23AM -0800, Stephen Hemminger wrote:
> 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");
>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 3/9] app/test: fix paren typo
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 1/9] app/test: do not duplicate loop variable Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 2/9] app/test: fix typo in address compare Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-22 0:34 ` fengchengwen
2024-11-21 18:23 ` [PATCH v4 4/9] app/test: avoid duplicate initialization Stephen Hemminger
` (6 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, ndabilpuram, stable, Bruce Richardson,
Akhil Goyal, Anoob Joseph
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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 3/9] app/test: fix paren typo
2024-11-21 18:23 ` [PATCH v4 3/9] app/test: fix paren typo Stephen Hemminger
@ 2024-11-22 0:34 ` fengchengwen
0 siblings, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-22 0:34 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: ndabilpuram, stable, Bruce Richardson, Akhil Goyal, Anoob Joseph
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
On 2024/11/22 2:23, 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>
> ---
> 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;
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 4/9] app/test: avoid duplicate initialization
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (2 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 3/9] app/test: fix paren typo Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 5/9] app/test: fix TLS zero length record Stephen Hemminger
` (5 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, jerin.jacob, Bruce Richardson, Abhinandan Gujjar
The event_dev_config initialization had duplicate assignments
to the same element. Change to use structure initialization
so that compiler will catch this type of bug.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
Cc: jerin.jacob@caviumnetworks.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
---
app/test/test_event_crypto_adapter.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/app/test/test_event_crypto_adapter.c b/app/test/test_event_crypto_adapter.c
index 9d38a66bfa..ab24e30a97 100644
--- a/app/test/test_event_crypto_adapter.c
+++ b/app/test/test_event_crypto_adapter.c
@@ -1154,21 +1154,17 @@ configure_cryptodev(void)
static inline void
evdev_set_conf_values(struct rte_event_dev_config *dev_conf,
- struct rte_event_dev_info *info)
+ const struct rte_event_dev_info *info)
{
- memset(dev_conf, 0, sizeof(struct rte_event_dev_config));
- dev_conf->dequeue_timeout_ns = info->min_dequeue_timeout_ns;
- dev_conf->nb_event_ports = NB_TEST_PORTS;
- dev_conf->nb_event_queues = NB_TEST_QUEUES;
- dev_conf->nb_event_queue_flows = info->max_event_queue_flows;
- dev_conf->nb_event_port_dequeue_depth =
- info->max_event_port_dequeue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_event_port_enqueue_depth =
- info->max_event_port_enqueue_depth;
- dev_conf->nb_events_limit =
- info->max_num_events;
+ *dev_conf = (struct rte_event_dev_config) {
+ .dequeue_timeout_ns = info->min_dequeue_timeout_ns,
+ .nb_event_ports = NB_TEST_PORTS,
+ .nb_event_queues = NB_TEST_QUEUES,
+ .nb_event_queue_flows = info->max_event_queue_flows,
+ .nb_event_port_dequeue_depth = info->max_event_port_dequeue_depth,
+ .nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth,
+ .nb_events_limit = info->max_num_events,
+ };
}
static int
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v4 5/9] app/test: fix TLS zero length record
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (3 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 4/9] app/test: avoid duplicate initialization Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-22 7:22 ` [EXTERNAL] " Anoob Joseph
2024-11-21 18:23 ` [PATCH v4 6/9] app/test: fix operator precedence bug Stephen Hemminger
` (4 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, vvelumuri, stable, Akhil Goyal, Fan Zhang,
Anoob Joseph
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v4 6/9] app/test: fix operator precedence bug
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (4 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 5/9] app/test: fix TLS zero length record Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-21 18:23 ` [PATCH v4 7/9] test/eal: fix core check in c flag test Stephen Hemminger
` (3 subsequent siblings)
9 siblings, 0 replies; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, stable, Bruce Richardson, Tyler Retzlaff
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 be 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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* [PATCH v4 7/9] test/eal: fix core check in c flag test
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (5 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 6/9] app/test: fix operator precedence bug Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-26 17:13 ` Aaron Conole
2024-11-21 18:23 ` [PATCH v4 8/9] app/test-pmd: remove redundant condition Stephen Hemminger
` (2 subsequent siblings)
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, msantana, stable, Bruce Richardson,
Tyler Retzlaff, Michael Santana, Aaron Conole, David Marchand
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>
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 7/9] test/eal: fix core check in c flag test
2024-11-21 18:23 ` [PATCH v4 7/9] test/eal: fix core check in c flag test Stephen Hemminger
@ 2024-11-26 17:13 ` Aaron Conole
0 siblings, 0 replies; 69+ messages in thread
From: Aaron Conole @ 2024-11-26 17:13 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev, msantana, stable, Bruce Richardson, Tyler Retzlaff,
Michael Santana, David Marchand
Stephen Hemminger <stephen@networkplumber.org> writes:
> 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>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
Acked-by: Aaron Conole <aconole@redhat.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");
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 8/9] app/test-pmd: remove redundant condition
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (6 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 7/9] test/eal: fix core check in c flag test Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-22 0:33 ` fengchengwen
2024-11-21 18:23 ` [PATCH v4 9/9] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
2024-11-26 22:33 ` [PATCH v4 0/9] Bug fixes for standalone tests Thomas Monjalon
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, haifeil, Bruce Richardson, Ajit Khaparde,
Aman Singh, Matan Azrad
The loop over policy actions will always exit when it sees
the flow end action, so the next check is redundant.
Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
Cc: haifeil@nvidia.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
app/test-pmd/config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 28d45568ac..4e7fb69183 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
- if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
+ if (act_n > 0)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
@@ -7338,4 +7338,3 @@ show_mcast_macs(portid_t port_id)
printf(" %s\n", buf);
}
}
-
--
2.45.2
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 8/9] app/test-pmd: remove redundant condition
2024-11-21 18:23 ` [PATCH v4 8/9] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-22 0:33 ` fengchengwen
0 siblings, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-22 0:33 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: haifeil, Bruce Richardson, Ajit Khaparde, Aman Singh, Matan Azrad
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
On 2024/11/22 2:23, Stephen Hemminger wrote:
> The loop over policy actions will always exit when it sees
> the flow end action, so the next check is redundant.
>
> Link: https://pvs-studio.com/en/blog/posts/cpp/1179/
>
> Fixes: f29fa2c59b85 ("app/testpmd: support policy actions per color")
> Cc: haifeil@nvidia.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> app/test-pmd/config.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 28d45568ac..4e7fb69183 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2288,7 +2288,7 @@ port_meter_policy_add(portid_t port_id, uint32_t policy_id,
> for (act_n = 0, start = act;
> act->type != RTE_FLOW_ACTION_TYPE_END; act++)
> act_n++;
> - if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
> + if (act_n > 0)
> policy.actions[i] = start;
> else
> policy.actions[i] = NULL;
> @@ -7338,4 +7338,3 @@ show_mcast_macs(portid_t port_id)
> printf(" %s\n", buf);
> }
> }
> -
^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 9/9] app/test-pmd: avoid potential outside of array reference
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (7 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 8/9] app/test-pmd: remove redundant condition Stephen Hemminger
@ 2024-11-21 18:23 ` Stephen Hemminger
2024-11-22 0:33 ` fengchengwen
2024-11-26 22:33 ` [PATCH v4 0/9] Bug fixes for standalone tests Thomas Monjalon
9 siblings, 1 reply; 69+ messages in thread
From: Stephen Hemminger @ 2024-11-21 18:23 UTC (permalink / raw)
To: dev
Cc: Stephen Hemminger, getelson, stable, Bruce Richardson, Ori Kam,
Aman Singh
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>
---
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
^ permalink raw reply [flat|nested] 69+ messages in thread* Re: [PATCH v4 9/9] app/test-pmd: avoid potential outside of array reference
2024-11-21 18:23 ` [PATCH v4 9/9] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
@ 2024-11-22 0:33 ` fengchengwen
0 siblings, 0 replies; 69+ messages in thread
From: fengchengwen @ 2024-11-22 0:33 UTC (permalink / raw)
To: Stephen Hemminger, dev
Cc: getelson, stable, Bruce Richardson, Ori Kam, Aman Singh
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
On 2024/11/22 2:23, 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>
> ---
> 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;
>
^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v4 0/9] Bug fixes for standalone tests
2024-11-21 18:23 ` [PATCH v4 0/9] Bug fixes for standalone tests Stephen Hemminger
` (8 preceding siblings ...)
2024-11-21 18:23 ` [PATCH v4 9/9] app/test-pmd: avoid potential outside of array reference Stephen Hemminger
@ 2024-11-26 22:33 ` Thomas Monjalon
9 siblings, 0 replies; 69+ messages in thread
From: Thomas Monjalon @ 2024-11-26 22:33 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev
21/11/2024 19:23, Stephen Hemminger:
> Recent blog post from PVS studio referenced lots bugs found by
> their analyzer against DPDK. This set addresses the ones in
> the test suite.
>
> v4 - rebase and drop code that was already fixed (removed)
>
> Stephen Hemminger (9):
> app/test: do not duplicate loop variable
> app/test: fix typo in address compare
> app/test: fix paren typo
> app/test: avoid duplicate initialization
> app/test: fix TLS zero length record
> app/test: fix operator precedence bug
> test/eal: fix core check in c flag test
> app/test-pmd: remove redundant condition
> app/test-pmd: avoid potential outside of array reference
Applied, thanks.
^ permalink raw reply [flat|nested] 69+ messages in thread