|SUCCESS| [v4,2/2] test: simplify spawning extra test processes

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163224

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-21

commit 20260421195227.1425625-2-bruce.richardson at intel.com
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: test: fix failing tests on FreeBSD

This patch addresses failing autotests on FreeBSD by skipping tests that
require secondary process support (which is not available on FreeBSD) and
fixing a crash in the debug autotest related to telemetry initialization.

>  app/test/test_debug.c           |  6 +++---
>  app/test/test_eal_flags.c       | 26 +++++++++++++-------------
>  app/test/test_external_mem.c    |  4 ++--
>  app/test/test_interrupts.c      |  2 +-
>  app/test/test_mp_secondary.c    |  4 ++--
>  app/test/test_pdump.c           |  4 ++++
>  app/test/test_timer_secondary.c |  4 ++--
>  7 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/app/test/test_debug.c b/app/test/test_debug.c
> index bfda4d1aa3..eda9c3e0e1 100644
> --- a/app/test/test_debug.c
> +++ b/app/test/test_debug.c
> @@ -154,15 +154,15 @@ test_debug(void)
>  	test_args[2] = "-l";
>  	test_args[3] = core;
>  
> -	if (rte_eal_has_hugepages()) {
> +	if (rte_eal_has_hugepages() && RTE_EXEC_ENV_IS_LINUX) {

In test_debug(), this condition now prevents hugepage-less operation on
FreeBSD even when hugepages are available. Should this be:

    if (rte_eal_has_hugepages())

with the else clause handling the no-hugepages case for both platforms?

>  		test_args[4] = "";
>  		test_args[5] = "";
>  		test_args[6] = "";
>  		test_args[7] = "";
>  	} else {
>  		test_args[4] = "--no-huge";
> -		test_args[5] = "-m";
> -		test_args[6] = "2048";
> +		test_args[5] = "-m2048";
> +		test_args[6] = "--no-telemetry";

This changes test_args[5] from two separate arguments ("-m" and "2048") 
to a single combined argument ("-m2048"). Does this affect command line
parsing? Also, is --no-telemetry valid on all supported DPDK platforms?

[ ... ]

> diff --git a/app/test/test_interrupts.c b/app/test/test_interrupts.c
> index 2eb876e65a..3a5be92cd7 100644
> --- a/app/test/test_interrupts.c
> +++ b/app/test/test_interrupts.c
> @@ -429,7 +429,7 @@ test_interrupt(void)
>  	int ret = -1;
>  	struct rte_intr_handle *test_intr_handle;
>  
> -	if (RTE_EXEC_ENV_IS_WINDOWS)
> +	if (RTE_EXEC_ENV_IS_WINDOWS || RTE_EXEC_ENV_IS_FREEBSD)
>  		return TEST_SKIPPED;

[ ... ]

> diff --git a/app/test/test_pdump.c b/app/test/test_pdump.c
> index 824caef2d4..64f1bbf21c 100644
> --- a/app/test/test_pdump.c
> +++ b/app/test/test_pdump.c
> @@ -198,6 +198,10 @@ int
>  test_pdump(void)
>  {
>  	int ret = 0;
> +
> +	if (RTE_EXEC_ENV_IS_FREEBSD)
> +		return TEST_SKIPPED;
> +
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {

In test_pdump(), the early return for FreeBSD bypasses the process type
check. Does this mean that if pdump is somehow invoked as a secondary
process on Linux, it would proceed past this check? The original code
structure would have caught that scenario.

[ ... ]

commit 20260421195227.1425625-3-bruce.richardson at intel.com
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: test: simplify spawning extra test processes

This patch simplifies spawning of secondary/additional test processes by
using argv[0] instead of querying procfs.

> diff --git a/app/test/process.h b/app/test/process.h
> index df43966a2a..3ee899dbc8 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -19,14 +19,6 @@
>  #include <rte_devargs.h>
>  #include <rte_eal.h>
>  
> -#ifdef RTE_EXEC_ENV_FREEBSD
> -#define self "curproc"
> -#define exe "file"
> -#else
> -#define self "self"
> -#define exe "exe"
> -#endif
> -

[ ... ]

> @@ -188,15 +179,8 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  		/* set the environment variable */
>  		if (setenv(RECURSIVE_ENV_VAR, env_value, 1) != 0)
>  			rte_panic("Cannot export environment variable\n");
> -
> -		strlcpy(path, "/proc/" self "/" exe, sizeof(path));
> -		if (execv(path, argv_cpy) < 0) {
> -			if (errno == ENOENT) {
> -				printf("Could not find '%s', is procfs mounted?\n",
> -						path);
> -			}
> +		if (execv(argv_cpy[0], argv_cpy) < 0)
>  			rte_panic("Cannot exec: %s\n", strerror(errno));
> -		}

In process_dup(), replacing the procfs-based path with argv_cpy[0]
assumes argv_cpy[0] contains a valid executable path. If the test was
invoked with a relative path and the working directory changed, would
execv() fail to find the binary? The original code using /proc/self/exe
was immune to such issues.

Also, does this code handle the case where argv[0] is just the program
name without any path component (i.e., found via PATH lookup)?