Re: [PATCH v4] app/test: append 'allow' parameters to secondary processes

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Sep 27, 2023 at 03:42:04AM +0000, Mingjin Ye wrote:
> The 'allow' parameters are not passed to the secondary process, and all
> devices will be loaded by default. When the primary process specifies
> the 'allow' parameters and does not specify all devices that use the
> vfio-pci driver, the secondary process will not load the devices as
> expected, which will return incorrect test results.
> 
> This patch fixes this issue by appending the 'allow' parameters to the
> secondary process.
> 
> Fixes: 086eb64db39e ("test/pdump: add unit test for pdump library")
> Fixes: 148f963fb532 ("xen: core library changes")
> Fixes: af75078fece3 ("first public release")
> Fixes: b8d5e544e73e ("test: add procfs error message for multi-process launch")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v4: Resolve patch conflicts and optimize code.

No issue with the idea of this fix, some impovement suggestions inline below.
Once addressed:

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

> ---
>  app/test/process.h | 60 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h
> index af7bc3e0de..1cdf28752e 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -18,6 +18,8 @@
>  
>  #include <rte_string_fns.h> /* strlcpy */
>  
> +#include <rte_devargs.h>
> +
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  #define self "curproc"
>  #define exe "file"
> @@ -34,6 +36,47 @@ extern uint16_t flag_for_send_pkts;
>  #endif
>  #endif
>  
> +#define PREFIX_ALLOW "--allow"
> +
> +static int
> +add_parameter_allow(char **argv, int max_capacity)
> +{
> +	struct rte_devargs *devargs;
> +	int count = 0;
> +	int name_length, data_length;
> +	char *dev;
> +	int malloc_size;
> +
> +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> +		if (count >= max_capacity)

Since you need two slots for each element, you need to check that count+1 <
max_capacity.

However, a better solution is to have just one flag for each dev-args. So
instead of returning an array with e.g.

["--allow", "a8:00.0", "--allow", "b8:00.0"]

instead use the "--allow=" syntax to merge elements, returning:

["--allow=a8:00.0", "--allow=b8:00.0"]

this would make your capacity check correct.

> +			return count;
> +
> +		name_length = strlen(devargs->name);
> +		if (name_length == 0)
> +			continue;
> +
> +		if (devargs->data != NULL)
> +			data_length = strlen(devargs->data);
> +		else
> +			data_length = 0;

Minor suggestion. Rather than having an else leg here, just move the
definitions of name_length and data_length inside the loop, so you can
zero-initialize it each time.

An even simpler option might be to just use a single malloc_size variable
as you don't need the others:

	malloc_size = strlen(devargs->name) + 1;
	if (malloc_size == 0)
		continue;

	malloc_size += strlen(PREFIX_ALLOW); /* which now has the "=" */
	if (devargs->data != NULL)
		malloc_size += strlen(devargs->data)

> +
> +		malloc_size = name_length + data_length + 1;
> +		dev = malloc(malloc_size);
> +
Check for NULL return from malloc.

> +		memcpy(dev, devargs->name, name_length);
> +		if (data_length > 0)
> +			memcpy(dev + name_length, devargs->data, data_length);
> +		memset(dev + malloc_size - 1, 0, 1);
> +
> +		*(argv + count) =  strdup(PREFIX_ALLOW);
> +		count++;
> +
> +		*(argv + count) = dev;
> +		count++;
> +	}
> +	return count;
> +}
> +
>  /*
>   * launches a second copy of the test process using the given argv parameters,
>   * which should include argv[0] as the process name. To identify in the
> @@ -44,7 +87,9 @@ static inline int
>  process_dup(const char *const argv[], int numargs, const char *env_value)
>  {
>  	int num;
> -	char *argv_cpy[numargs + 1];
> +	char **argv_cpy;
> +	unsigned int allow_num;
> +	unsigned int argv_num;
>  	int i, status;
>  	char path[32];
>  #ifdef RTE_LIB_PDUMP
> @@ -58,12 +103,15 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  	if (pid < 0)
>  		return -1;
>  	else if (pid == 0) {
> +		allow_num = rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
> +		argv_num = numargs + allow_num * 2 + 1;
> +		argv_cpy = malloc(argv_num * sizeof(char *));
>  		/* make a copy of the arguments to be passed to exec */
>  		for (i = 0; i < numargs; i++)
>  			argv_cpy[i] = strdup(argv[i]);
> -		argv_cpy[i] = NULL;
>  		num = numargs;
> -
> +		num += add_parameter_allow(&argv_cpy[i], allow_num * 2);

If merging flags, you can remove the *2 here and in the argv_num = line.

> +		argv_cpy[argv_num - 1] = NULL;
>  #ifdef RTE_EXEC_ENV_LINUX
>  		{
>  			const char *procdir = "/proc/" self "/fd/";
> @@ -131,6 +179,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  			}
>  			rte_panic("Cannot exec: %s\n", strerror(errno));
>  		}
> +
> +		for (i = 0; i < num; i++) {
> +			if (argv_cpy[i] != NULL)
> +				free(argv_cpy[i]);
> +		}
> +		free(argv_cpy);
>  	}
>  	/* parent process does a wait */
>  #ifdef RTE_LIB_PDUMP
> -- 
> 2.25.1
>