|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

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

This commit fixes several test failures on FreeBSD by skipping tests that
require secondary process support (not available on FreeBSD) and correcting
the debug test to disable telemetry when running without hugepages.

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

Can the logic here be simplified? The condition now reads as "if we have
hugepages AND we are on Linux" which seems overly specific. What happens
on FreeBSD with hugepages?

[ ... ]

> 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 pattern repeats throughout the file. These functions now skip on both
Windows and FreeBSD. Does this change align with the actual platform
capabilities? For instance, are these flags truly Linux-specific, or were
they previously working on FreeBSD?

[ ... ]

> diff --git a/app/test/test_external_mem.c b/app/test/test_external_mem.c
> index 1c977b749b..53300983ed 100644
> --- a/app/test/test_external_mem.c
> +++ b/app/test/test_external_mem.c
> @@ -10,11 +10,11 @@
>  #include <string.h>
>  #include <fcntl.h>
>  
> -#ifdef RTE_EXEC_ENV_WINDOWS
> +#ifndef RTE_EXEC_ENV_LINUX
>  static int
>  test_external_mem(void)
>  {
> -	printf("external_mem not supported on Windows, skipping test\n");
> +	printf("external_mem only supported on Linux, skipping test\n");
>  	return TEST_SKIPPED;

Same question - was external memory support previously available on FreeBSD
through the non-skipped code path?

[ ... ]

> diff --git a/app/test/test_mp_secondary.c b/app/test/test_mp_secondary.c
> index 218eacb3bf..d89419c253 100644
> --- a/app/test/test_mp_secondary.c
> +++ b/app/test/test_mp_secondary.c
> @@ -15,11 +15,11 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> -#ifdef RTE_EXEC_ENV_WINDOWS
> +#ifndef RTE_EXEC_ENV_LINUX
>  int
>  test_mp_secondary(void)
>  {
> -	printf("mp_secondary not supported on Windows, skipping test\n");
> +	printf("Multiprocess support only present on Linux, skipping test\n");

The commit message and Bugzilla ID 761 mention lack of secondary process
support on FreeBSD as the main issue. This change confirms that
understanding. However, does FreeBSD truly have no multiprocess support, or
is the test infrastructure missing? The broader implications of disabling
this for FreeBSD should be clear.

[ ... ]

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

This check happens before the process type check. Is this the right
ordering? The comment about secondary process support suggests this should
perhaps be checking for multiprocess capability rather than OS type. Would
a more general check be clearer?

[ ... ]

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

This commit simplifies test process spawning by using argv[0] instead of
querying procfs, removing the FreeBSD dependency on /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

[ ... ]

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

Does this code validate that argv_cpy[0] is an absolute path? The commit
message states "The unit tests should never be running in a way that leads
to argv[0] not being the actual test binary" but what happens if someone
runs the test via PATH lookup without an absolute or relative path? Does
execv() handle relative paths correctly in all cases where the original
procfs approach would work?

The removal of the ENOENT-specific error message also makes debugging
harder. If execv() fails, users will only see a generic exec failure
message. Was this diagnostic information valuable for troubleshooting?