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

  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.
---
 app/test/process.h | 60 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)
  

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Wednesday, September 27, 2023 11:42 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; Ye, MingjinX <mingjinx.ye@intel.com>;
> stable@dpdk.org
> Subject: [PATCH v4] app/test: append 'allow' parameters to secondary
> processes
> 
> 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>
> ---
Verified multiple pcis with the '-a' parameter.
Tested-by: Zhimin Huang <zhiminx.huang@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
>