|SUCCESS| [v3,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/162953

_AI Code Review_

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

Commit: (git sha not accessible from patch file alone)
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: [PATCH v3 1/2] test: fix failing tests on FreeBSD

This patch disables tests lacking secondary process support on FreeBSD and
fixes a crash in the debug autotest when telemetry is not disabled for the
spawned process.

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

Does this logic correctly handle all cases? The condition now only allows
empty test_args on Linux with hugepages. What happens on FreeBSD with
hugepages - does it fall through to the --no-huge path even though
hugepages are available?

>  		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";

The comment in the commit message says this fixes telemetry crashes on
FreeBSD, but can this code path be reached on Linux as well when there
are no hugepages? If so, does disabling telemetry have any unwanted
side effects on Linux testing?

[ ... ]

> diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c
> index b3a8d0ae6f..12280e7a39 100644
> --- a/app/test/test_eal_flags.c
> +++ b/app/test/test_eal_flags.c
> @@ -10,88 +10,88 @@
>  #include <string.h>
>  #include <stdarg.h>
>  
> -#ifdef RTE_EXEC_ENV_WINDOWS
> +#ifndef RTE_EXEC_ENV_LINUX
>  static int
>  test_missing_c_flag(void)
>  {
> -	printf("missing_c_flag not supported on Windows, skipping test\n");
> +	printf("missing_c_flag only supported on Linux, skipping test\n");

This changes behavior for Windows - the tests will now be skipped there
too when previously they had Windows-specific messages. Is this
intentional, or should Windows handling be preserved separately?

[ ... ]

> 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;

Does this skip hide a real portability issue? Should there be a comment
explaining why FreeBSD cannot support this test, similar to how other
platforms document their limitations?

[ ... ]

---

Commit: (git sha not accessible from patch file alone)
Author: Bruce Richardson <bruce.richardson at intel.com>
Subject: [PATCH v3 2/2] test: simplify spawning extra test processes

This patch removes the dependency on procfs by using argv[0] instead of
querying /proc/self/exe or /proc/curproc/file.

> 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

[ ... ]

> @@ -132,7 +123,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  
>  #ifdef RTE_EXEC_ENV_LINUX
>  		{
> -			const char *procdir = "/proc/" self "/fd/";
> +			const char *procdir = "/proc/self/fd/";
>  			struct dirent *dirent;
>  			char *endptr;
>  			int fd, fdir;

[ ... ]

> @@ -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));
> -		}

The commit message states that unit tests should never run in a way that
leads to argv[0] not being the actual test binary. However, can argv[0]
be a relative path? If the test binary was invoked with a relative path
and the working directory changes before this execv(), does this code fail
to find the binary?