[PATCH] child process synchronization NIC startup parameters

[PATCH] child process synchronization NIC startup parameters

[Kaisen You]

In meson_test, because the child process does not synchronize
the NIC startup parameters of the parent process at startup,
it uses all NICs bound by vfio as startup parameters by default,
and an exception occurs in the subsequent hugefile check,
causing the test to fail. Synchronize the NIC startup parameters
of the parent process to the child process to solve this problem.

Fixes: 786b29255c49 ("test: fix file prefix discovery")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>
---
 app/test/process.h | 80 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 3 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..c8a15e8989 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -15,9 +15,11 @@
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
+#include <fcntl.h>
 
 #include <rte_string_fns.h> /* strlcpy */
 
+#define MAX_EXTRA_ARGS 32
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
 #define exe "file"
@@ -34,6 +36,50 @@ extern uint16_t flag_for_send_pkts;
 #endif
 #endif
 
+/* get cmdline form PID. Read process info form /proc/$PID. */
+static char *get_cmdline_from_pid(pid_t pid, char *buf, int len)
+{
+	char filename[PATH_MAX];
+	char *name = NULL;
+	int fd;
+	int ret;
+
+	if (pid < 1 || buf == NULL || len < 0) {
+		printf("%s: illegal param\n", __func__);
+		return NULL;
+	}
+
+	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
+	memset(buf, 0, len);
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		perror("open:");
+		return NULL;
+	}
+	ret = read(fd, buf, len);
+	close(fd);
+
+	if (ret < 0)
+		return NULL;
+
+	if (buf[ret-1] == '\n')
+		buf[--ret] = 0;
+
+	name = buf;
+	while (ret) {
+		if (((unsigned char)*name) < ' ')
+			*name = ' ';
+		name++;
+		ret--;
+	}
+	*name = 0;
+
+	if (buf[0])
+		return buf;
+
+	return NULL;
+}
+
 /*
  * 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,9 +90,15 @@ static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
-	char *argv_cpy[numargs + 1];
-	int i, status;
+	char *argv_cpy[MAX_EXTRA_ARGS];
+	int i, status, n, s, j;
 	char path[32];
+	char buf[1024];
+	char *token;
+	char str_1[] = "-a";
+	char str_2[] = " ";
+	char *argv_str[MAX_EXTRA_ARGS];
+	char *argv_copy[MAX_EXTRA_ARGS];
 #ifdef RTE_LIB_PDUMP
 #ifdef RTE_NET_RING
 	pthread_t thread;
@@ -113,10 +165,32 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 			closedir(dir);
 		}
 #endif
+		/* Add the -a parameter to the child process start parameter */
+		get_cmdline_from_pid(getppid(), buf, 1024);
+		token = strtok(buf, str_2);
+		argv_str[0] = strdup(token);
+		n = 0;
+		j = 0;
+		while (token != NULL) {
+			n = n + 1;
+			argv_str[n] = strdup(token);
+			token = strtok(NULL, str_2);
+		}
+		for (s = 0; s < n; s++) {
+			if (strcmp(argv_str[s], str_1) == 0 ||
+				strcmp(argv_str[s + 1], str_1) == 0) {
+				argv_copy[j] = strdup(argv_str[s + 1]);
+				j++;
+			}
+		}
+		for (s = 0; s < j; s++)
+			argv_cpy[numargs + s] = strdup(argv_copy[s]);
+
 		printf("Running binary with argv[]:");
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num + j; i++)
 			printf("'%s' ", argv_cpy[i]);
 		printf("\n");
+		argv_cpy[numargs + j] = NULL;
 		fflush(stdout);
 
 		/* set the environment variable */
-- 
2.25.1

RE: [PATCH] child process synchronization NIC startup parameters

[Yang, Qiming]

Hi, kaisen
Your patch missed prefix app/test.

> -----Original Message-----
> From: You, KaisenX <kaisenx.you@intel.com>
> Sent: Wednesday, July 5, 2023 5:35 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; You, KaisenX <kaisenx.you@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] child process synchronization NIC startup parameters
> 
> In meson_test, because the child process does not synchronize the NIC
> startup parameters of the parent process at startup, it uses all NICs bound by
> vfio as startup parameters by default, and an exception occurs in the
> subsequent hugefile check, causing the test to fail. Synchronize the NIC
> startup parameters of the parent process to the child process to solve this
> problem.
> 
> Fixes: 786b29255c49 ("test: fix file prefix discovery")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> ---
>  app/test/process.h | 80
> ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h index
> 1f073b9c5c..c8a15e8989 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -15,9 +15,11 @@
>  #include <string.h> /* strerror */
>  #include <unistd.h> /* readlink */
>  #include <dirent.h>
> +#include <fcntl.h>
> 
>  #include <rte_string_fns.h> /* strlcpy */
> 
> +#define MAX_EXTRA_ARGS 32
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  #define self "curproc"
>  #define exe "file"
> @@ -34,6 +36,50 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> 
> +/* get cmdline form PID. Read process info form /proc/$PID. */ static
> +char *get_cmdline_from_pid(pid_t pid, char *buf, int len) {
> +	char filename[PATH_MAX];
> +	char *name = NULL;
> +	int fd;
> +	int ret;
> +
> +	if (pid < 1 || buf == NULL || len < 0) {
> +		printf("%s: illegal param\n", __func__);
> +		return NULL;
> +	}
> +
> +	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
> +	memset(buf, 0, len);
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0) {
> +		perror("open:");
> +		return NULL;
> +	}
> +	ret = read(fd, buf, len);
> +	close(fd);
> +
> +	if (ret < 0)
> +		return NULL;
> +
> +	if (buf[ret-1] == '\n')
> +		buf[--ret] = 0;
> +
> +	name = buf;
> +	while (ret) {
> +		if (((unsigned char)*name) < ' ')
> +			*name = ' ';
> +		name++;
> +		ret--;
> +	}
> +	*name = 0;
> +
> +	if (buf[0])
> +		return buf;
> +
> +	return NULL;
> +}
> +
>  /*
>   * 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,9 +90,15 @@ static inline int  process_dup(const char *const argv[], int
> numargs, const char *env_value)  {
>  	int num;
> -	char *argv_cpy[numargs + 1];
> -	int i, status;
> +	char *argv_cpy[MAX_EXTRA_ARGS];
> +	int i, status, n, s, j;
>  	char path[32];
> +	char buf[1024];
> +	char *token;
> +	char str_1[] = "-a";
> +	char str_2[] = " ";
> +	char *argv_str[MAX_EXTRA_ARGS];
> +	char *argv_copy[MAX_EXTRA_ARGS];
>  #ifdef RTE_LIB_PDUMP
>  #ifdef RTE_NET_RING
>  	pthread_t thread;
> @@ -113,10 +165,32 @@ process_dup(const char *const argv[], int numargs,
> const char *env_value)
>  			closedir(dir);
>  		}
>  #endif
> +		/* Add the -a parameter to the child process start parameter
> */
> +		get_cmdline_from_pid(getppid(), buf, 1024);
> +		token = strtok(buf, str_2);
> +		argv_str[0] = strdup(token);
> +		n = 0;
> +		j = 0;
> +		while (token != NULL) {
> +			n = n + 1;
> +			argv_str[n] = strdup(token);
> +			token = strtok(NULL, str_2);
> +		}
> +		for (s = 0; s < n; s++) {
> +			if (strcmp(argv_str[s], str_1) == 0 ||
> +				strcmp(argv_str[s + 1], str_1) == 0) {
> +				argv_copy[j] = strdup(argv_str[s + 1]);
> +				j++;
> +			}
> +		}
> +		for (s = 0; s < j; s++)
> +			argv_cpy[numargs + s] = strdup(argv_copy[s]);
> +
>  		printf("Running binary with argv[]:");
> -		for (i = 0; i < num; i++)
> +		for (i = 0; i < num + j; i++)
>  			printf("'%s' ", argv_cpy[i]);
>  		printf("\n");
> +		argv_cpy[numargs + j] = NULL;
>  		fflush(stdout);
> 
>  		/* set the environment variable */
> --
> 2.25.1

RE: [PATCH] child process synchronization NIC startup parameters

[You, KaisenX]

> -----Original Message-----
> From: Yang, Qiming <qiming.yang@intel.com>
> Sent: 2023年7月6日 10:17
> To: You, KaisenX <kaisenx.you@intel.com>; dev@dpdk.org
> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] child process synchronization NIC startup parameters
> 
> Hi, kaisen
> Your patch missed prefix app/test.

Thank you for your reply, I will send v2 to fix this problem as soon as possible.
> 
> > -----Original Message-----
> > From: You, KaisenX <kaisenx.you@intel.com>
> > Sent: Wednesday, July 5, 2023 5:35 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > <yidingx.zhou@intel.com>; You, KaisenX <kaisenx.you@intel.com>;
> > stable@dpdk.org
> > Subject: [PATCH] child process synchronization NIC startup parameters
> >
> > In meson_test, because the child process does not synchronize the NIC
> > startup parameters of the parent process at startup, it uses all NICs
> > bound by vfio as startup parameters by default, and an exception
> > occurs in the subsequent hugefile check, causing the test to fail.
> > Synchronize the NIC startup parameters of the parent process to the
> > child process to solve this problem.
> >
> > Fixes: 786b29255c49 ("test: fix file prefix discovery")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> > ---
> >  app/test/process.h | 80
> > ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 77 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test/process.h b/app/test/process.h index
> > 1f073b9c5c..c8a15e8989 100644
> > --- a/app/test/process.h
> > +++ b/app/test/process.h
> > @@ -15,9 +15,11 @@
> >  #include <string.h> /* strerror */
> >  #include <unistd.h> /* readlink */
> >  #include <dirent.h>
> > +#include <fcntl.h>
> >
> >  #include <rte_string_fns.h> /* strlcpy */
> >
> > +#define MAX_EXTRA_ARGS 32
> >  #ifdef RTE_EXEC_ENV_FREEBSD
> >  #define self "curproc"
> >  #define exe "file"
> > @@ -34,6 +36,50 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> >
> > +/* get cmdline form PID. Read process info form /proc/$PID. */ static
> > +char *get_cmdline_from_pid(pid_t pid, char *buf, int len) {
> > +	char filename[PATH_MAX];
> > +	char *name = NULL;
> > +	int fd;
> > +	int ret;
> > +
> > +	if (pid < 1 || buf == NULL || len < 0) {
> > +		printf("%s: illegal param\n", __func__);
> > +		return NULL;
> > +	}
> > +
> > +	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
> > +	memset(buf, 0, len);
> > +	fd = open(filename, O_RDONLY);
> > +	if (fd < 0) {
> > +		perror("open:");
> > +		return NULL;
> > +	}
> > +	ret = read(fd, buf, len);
> > +	close(fd);
> > +
> > +	if (ret < 0)
> > +		return NULL;
> > +
> > +	if (buf[ret-1] == '\n')
> > +		buf[--ret] = 0;
> > +
> > +	name = buf;
> > +	while (ret) {
> > +		if (((unsigned char)*name) < ' ')
> > +			*name = ' ';
> > +		name++;
> > +		ret--;
> > +	}
> > +	*name = 0;
> > +
> > +	if (buf[0])
> > +		return buf;
> > +
> > +	return NULL;
> > +}
> > +
> >  /*
> >   * 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,9 +90,15 @@ static inline int  process_dup(const char *const
> > argv[], int numargs, const char *env_value)  {
> >  	int num;
> > -	char *argv_cpy[numargs + 1];
> > -	int i, status;
> > +	char *argv_cpy[MAX_EXTRA_ARGS];
> > +	int i, status, n, s, j;
> >  	char path[32];
> > +	char buf[1024];
> > +	char *token;
> > +	char str_1[] = "-a";
> > +	char str_2[] = " ";
> > +	char *argv_str[MAX_EXTRA_ARGS];
> > +	char *argv_copy[MAX_EXTRA_ARGS];
> >  #ifdef RTE_LIB_PDUMP
> >  #ifdef RTE_NET_RING
> >  	pthread_t thread;
> > @@ -113,10 +165,32 @@ process_dup(const char *const argv[], int
> > numargs, const char *env_value)
> >  			closedir(dir);
> >  		}
> >  #endif
> > +		/* Add the -a parameter to the child process start parameter
> > */
> > +		get_cmdline_from_pid(getppid(), buf, 1024);
> > +		token = strtok(buf, str_2);
> > +		argv_str[0] = strdup(token);
> > +		n = 0;
> > +		j = 0;
> > +		while (token != NULL) {
> > +			n = n + 1;
> > +			argv_str[n] = strdup(token);
> > +			token = strtok(NULL, str_2);
> > +		}
> > +		for (s = 0; s < n; s++) {
> > +			if (strcmp(argv_str[s], str_1) == 0 ||
> > +				strcmp(argv_str[s + 1], str_1) == 0) {
> > +				argv_copy[j] = strdup(argv_str[s + 1]);
> > +				j++;
> > +			}
> > +		}
> > +		for (s = 0; s < j; s++)
> > +			argv_cpy[numargs + s] = strdup(argv_copy[s]);
> > +
> >  		printf("Running binary with argv[]:");
> > -		for (i = 0; i < num; i++)
> > +		for (i = 0; i < num + j; i++)
> >  			printf("'%s' ", argv_cpy[i]);
> >  		printf("\n");
> > +		argv_cpy[numargs + j] = NULL;
> >  		fflush(stdout);
> >
> >  		/* set the environment variable */
> > --
> > 2.25.1

Re: [PATCH] child process synchronization NIC startup parameters

[Stephen Hemminger]

On Wed,  5 Jul 2023 17:35:14 +0800
Kaisen You <kaisenx.you@intel.com> wrote:

> In meson_test, because the child process does not synchronize
> the NIC startup parameters of the parent process at startup,
> it uses all NICs bound by vfio as startup parameters by default,
> and an exception occurs in the subsequent hugefile check,
> causing the test to fail. Synchronize the NIC startup parameters
> of the parent process to the child process to solve this problem.
> 
> Fixes: 786b29255c49 ("test: fix file prefix discovery")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>

Not portable outside of Linux.
Seems really bug prone to have test diving into /proc to find
command line of parent. Couldn't this be passed one of many other
ways into child?

[PATCH v2] app/test:subprocess synchronization of parameters

[Kaisen You]

In meson_test, because the child process does not synchronize
the NIC startup parameters of the parent process at startup,
it uses all NICs bound by vfio as startup parameters by default,
and an exception occurs in the subsequent hugefile check,
causing the test to fail. Synchronize the NIC startup parameters
of the parent process to the child process to solve this problem.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>

---
Changes since v1:
- change the patch title to modify the way child
  processes get NIC parameters,
---
 app/test/process.h | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..6a8505b80c 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -15,9 +15,12 @@
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
+#include <private.h>
 
 #include <rte_string_fns.h> /* strlcpy */
 
+#define MAX_EXTRA_ARGS 32
+#define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
 #define exe "file"
@@ -33,7 +36,6 @@ extern void *send_pkts(void *empty);
 extern uint16_t flag_for_send_pkts;
 #endif
 #endif
-
 /*
  * 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,9 +46,13 @@ static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
-	char *argv_cpy[numargs + 1];
-	int i, status;
+	char *argv_cpy[MAX_EXTRA_ARGS];
+	int i, status, s;
 	char path[32];
+	struct rte_pci_device *dev = NULL;
+	char type[MAX_EXTRA_ARGS];
+	char *argv_str[MAX_EXTRA_ARGS];
+	char str_1[] = "-a";
 #ifdef RTE_LIB_PDUMP
 #ifdef RTE_NET_RING
 	pthread_t thread;
@@ -113,10 +119,23 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 			closedir(dir);
 		}
 #endif
+		s = -1;
+		argv_str[0] = strdup(str_1);
+		FOREACH_DEVICE_ON_PCIBUS(dev) {
+			s = s + 2;
+			sprintf(type, PCI_PRI_FMT, dev->addr.domain,
+			dev->addr.bus, dev->addr.devid, dev->addr.function);
+			argv_str[s - 1] = strdup(str_1);
+			argv_str[s] = strdup(type);
+		}
+		for (i = 0; i < s + 1; i++)
+			argv_cpy[num + i] = strdup(argv_str[i]);
+
 		printf("Running binary with argv[]:");
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num + s + 1; i++)
 			printf("'%s' ", argv_cpy[i]);
 		printf("\n");
+		argv_cpy[numargs + s + 1] = NULL;
 		fflush(stdout);
 
 		/* set the environment variable */
-- 
2.25.1

[PATCH v2] app/test:subprocess synchronization of parameters

[Kaisen You]

In meson_test, because the child process does not synchronize
the NIC startup parameters of the parent process at startup,
it uses all NICs bound by vfio as startup parameters by default,
and an exception occurs in the subsequent hugefile check,
causing the test to fail. Synchronize the NIC startup parameters
of the parent process to the child process to solve this problem.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>

---
Changes since v1:
- change the patch title to modify the way child
  processes get NIC parameters,
---
 app/test/process.h | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..6a8505b80c 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -15,9 +15,12 @@
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
+#include "private.h"
 
 #include <rte_string_fns.h> /* strlcpy */
 
+#define MAX_EXTRA_ARGS 32
+#define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
 #define exe "file"
@@ -33,7 +36,6 @@ extern void *send_pkts(void *empty);
 extern uint16_t flag_for_send_pkts;
 #endif
 #endif
-
 /*
  * 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,9 +46,13 @@ static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
-	char *argv_cpy[numargs + 1];
-	int i, status;
+	char *argv_cpy[MAX_EXTRA_ARGS];
+	int i, status, s;
 	char path[32];
+	struct rte_pci_device *dev = NULL;
+	char type[MAX_EXTRA_ARGS];
+	char *argv_str[MAX_EXTRA_ARGS];
+	char str_1[] = "-a";
 #ifdef RTE_LIB_PDUMP
 #ifdef RTE_NET_RING
 	pthread_t thread;
@@ -113,10 +119,23 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 			closedir(dir);
 		}
 #endif
+		s = -1;
+		argv_str[0] = strdup(str_1);
+		FOREACH_DEVICE_ON_PCIBUS(dev) {
+			s = s + 2;
+			sprintf(type, PCI_PRI_FMT, dev->addr.domain,
+			dev->addr.bus, dev->addr.devid, dev->addr.function);
+			argv_str[s - 1] = strdup(str_1);
+			argv_str[s] = strdup(type);
+		}
+		for (i = 0; i < s + 1; i++)
+			argv_cpy[num + i] = strdup(argv_str[i]);
+
 		printf("Running binary with argv[]:");
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num + s + 1; i++)
 			printf("'%s' ", argv_cpy[i]);
 		printf("\n");
+		argv_cpy[numargs + s + 1] = NULL;
 		fflush(stdout);
 
 		/* set the environment variable */
-- 
2.25.1

[PATCH v2] app/test:subprocess synchronization of parameters

[Kaisen You]

In meson_test, because the child process does not synchronize
the NIC startup parameters of the parent process at startup,
it uses all NICs bound by vfio as startup parameters by default,
and an exception occurs in the subsequent hugefile check,
causing the test to fail. Synchronize the NIC startup parameters
of the parent process to the child process to solve this problem.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>

---
Changes since v1:
- change the patch title to modify the way child
  processes get NIC parameters
---
 app/test/process.h | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..76a1646b24 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -15,9 +15,12 @@
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
+#include "../../drivers/bus/pci/private.h"
 
 #include <rte_string_fns.h> /* strlcpy */
 
+#define MAX_EXTRA_ARGS 32
+#define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
 #define exe "file"
@@ -33,7 +36,6 @@ extern void *send_pkts(void *empty);
 extern uint16_t flag_for_send_pkts;
 #endif
 #endif
-
 /*
  * 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,9 +46,13 @@ static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
-	char *argv_cpy[numargs + 1];
-	int i, status;
+	char *argv_cpy[MAX_EXTRA_ARGS];
+	int i, status, s;
 	char path[32];
+	struct rte_pci_device *dev = NULL;
+	char type[MAX_EXTRA_ARGS];
+	char *argv_str[MAX_EXTRA_ARGS];
+	char str_1[] = "-a";
 #ifdef RTE_LIB_PDUMP
 #ifdef RTE_NET_RING
 	pthread_t thread;
@@ -113,10 +119,23 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 			closedir(dir);
 		}
 #endif
+		s = -1;
+		argv_str[0] = strdup(str_1);
+		FOREACH_DEVICE_ON_PCIBUS(dev) {
+			s = s + 2;
+			sprintf(type, PCI_PRI_FMT, dev->addr.domain,
+			dev->addr.bus, dev->addr.devid, dev->addr.function);
+			argv_str[s - 1] = strdup(str_1);
+			argv_str[s] = strdup(type);
+		}
+		for (i = 0; i < s + 1; i++)
+			argv_cpy[num + i] = strdup(argv_str[i]);
+
 		printf("Running binary with argv[]:");
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num + s + 1; i++)
 			printf("'%s' ", argv_cpy[i]);
 		printf("\n");
+		argv_cpy[numargs + s + 1] = NULL;
 		fflush(stdout);
 
 		/* set the environment variable */
-- 
2.25.1

[PATCH v3] app/test: secondary process passes allow parameters

[Mingjin Ye]

In EAL related test cases, the allow parameters are not passed to
the secondary process, resulting in unexpected NICs being loaded.

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>
---
v3:new solution.
---
 app/test/process.h | 52 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..57d58309d8 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,44 @@ extern uint16_t flag_for_send_pkts;
 #endif
 #endif
 
+#define PREFIX_A "-a"
+
+static int
+add_parameter_a(char **argv, int max_capacity)
+{
+	struct rte_devargs *devargs;
+	int count = 0;
+	int name_length, data_length;
+	char *dev;
+	int malloc_szie;
+
+	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
+		if (count >= max_capacity)
+			return count;
+
+		name_length = strlen(devargs->name);
+		if (devargs->data != NULL)
+			data_length = strlen(devargs->data);
+		else
+			data_length = 0;
+
+		malloc_szie = name_length + data_length + 1;
+		dev = malloc(malloc_szie);
+
+		strncpy(dev, devargs->name, name_length);
+		if (data_length > 0)
+			strncpy(dev + name_length, devargs->data, data_length);
+		memset(dev + malloc_szie - 1, 0, 1);
+
+		*(argv + count) =  strdup(PREFIX_A);
+		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 +84,7 @@ 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[numargs + RTE_MAX_ETHPORTS * 2 + 1];
 	int i, status;
 	char path[32];
 #ifdef RTE_LIB_PDUMP
@@ -58,11 +98,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 	if (pid < 0)
 		return -1;
 	else if (pid == 0) {
+		memset(argv_cpy, 0, numargs + RTE_MAX_ETHPORTS * 2 + 1);
 		/* 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_a(&argv_cpy[i], RTE_MAX_ETHPORTS * 2);
 
 #ifdef RTE_EXEC_ENV_LINUX
 		{
@@ -131,6 +172,13 @@ 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]);
+				argv_cpy[i] = NULL;
+			}
+		}
 	}
 	/* parent process does a wait */
 #ifdef RTE_LIB_PDUMP
-- 
2.25.1

Re: [PATCH v3] app/test: secondary process passes allow parameters

[David Marchand]

On Mon, Sep 25, 2023 at 11:54 AM Mingjin Ye <mingjinx.ye@intel.com> wrote:
>
> In EAL related test cases, the allow parameters are not passed to
> the secondary process, resulting in unexpected NICs being loaded.
>
> This patch fixes this issue by appending the allow parameters to
> the secondary process.

This patch looks wrong.

Can you provide a description of the issue?



-- 
David Marchand

RE: [PATCH v3] app/test: secondary process passes allow parameters

[Ye, MingjinX]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday, September 25, 2023 5:58 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v3] app/test: secondary process passes allow
> parameters
> 
> On Mon, Sep 25, 2023 at 11:54 AM Mingjin Ye <mingjinx.ye@intel.com>
> wrote:
> >
> > In EAL related test cases, the allow parameters are not passed to the
> > secondary process, resulting in unexpected NICs being loaded.
> >
> > This patch fixes this issue by appending the allow parameters to the
> > secondary process.
> 
> This patch looks wrong.
> 
> Can you provide a description of the issue?

CI/Checkpatch a warning has appeared. Marked as Superseded.
The new patch will replace it.

Re: [PATCH v3] app/test: secondary process passes allow parameters

[David Marchand]

On Mon, Sep 25, 2023 at 12:09 PM Ye, MingjinX <mingjinx.ye@intel.com> wrote:
> > > In EAL related test cases, the allow parameters are not passed to the
> > > secondary process, resulting in unexpected NICs being loaded.
> > >
> > > This patch fixes this issue by appending the allow parameters to the
> > > secondary process.
> >
> > This patch looks wrong.
> >
> > Can you provide a description of the issue?
>
> CI/Checkpatch a warning has appeared. Marked as Superseded.
> The new patch will replace it.

Rather than a new patch because of some compilation issue on the
implementation, please explain what you are trying to fix.
It may avoid wasting time.


-- 
David Marchand

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

[Mingjin Ye]

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

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)
+			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;
+
+		malloc_size = name_length + data_length + 1;
+		dev = malloc(malloc_size);
+
+		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);
+		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

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

[Huang, ZhiminX]

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

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

[Bruce Richardson]

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
> 

[PATCH v5] app/test: secondary process passes allow parameters

[Mingjin Ye]

In EAL related test cases, the allow parameters are not passed to
the secondary process, resulting in unexpected NICs being loaded.

This patch fixes this issue by appending the allow parameters to
the secondary process.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v5: Optimized.
---
 app/test/process.h | 74 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 4 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index af7bc3e0de..f8beb3c36f 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,57 @@ 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;
+	char *dev;
+	int malloc_size;
+	int allow_size = strlen(PREFIX_ALLOW);
+	int offset;
+
+	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
+		int name_length = 0;
+		int data_length = 0;
+
+		if (count >= max_capacity)
+			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;
+
+		malloc_size = allow_size + name_length + data_length + 1;
+		dev = malloc(malloc_size);
+		if (!dev)
+			return count;
+
+		offset = 0;
+		memcpy(dev + offset, PREFIX_ALLOW, allow_size);
+		offset += allow_size;
+		memcpy(dev + offset, devargs->name, name_length);
+		offset += name_length;
+		if (data_length > 0) {
+			memcpy(dev + offset, devargs->data, data_length);
+			offset += data_length;
+		}
+		memset(dev + offset, 0x00, 1);
+
+		*(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 +97,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;
+	int allow_num;
+	int argv_num;
 	int i, status;
 	char path[32];
 #ifdef RTE_LIB_PDUMP
@@ -58,12 +113,17 @@ 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 + 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);
+		if (num != allow_num)
+			rte_panic("Fill allow parameter incomplete\n");
+		num += numargs;
+		argv_cpy[argv_num - 1] = NULL;
 #ifdef RTE_EXEC_ENV_LINUX
 		{
 			const char *procdir = "/proc/" self "/fd/";
@@ -131,6 +191,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

Re: [PATCH v5] app/test: secondary process passes allow parameters

[Bruce Richardson]

On Fri, Nov 10, 2023 at 10:30:12AM +0000, Mingjin Ye wrote:
> In EAL related test cases, the allow parameters are not passed to
> the secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to
> the secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Some additional comments below. Sorry I missed out on them in my first
review.

/Bruce

> ---
> v5: Optimized.
> ---
>  app/test/process.h | 74 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 70 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h
> index af7bc3e0de..f8beb3c36f 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,57 @@ 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;
> +	char *dev;
> +	int malloc_size;
> +	int allow_size = strlen(PREFIX_ALLOW);
> +	int offset;
> +
> +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> +		int name_length = 0;
> +		int data_length = 0;
> +
> +		if (count >= max_capacity)
> +			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;
> +
> +		malloc_size = allow_size + name_length + data_length + 1;
> +		dev = malloc(malloc_size);
> +		if (!dev)
> +			return count;
> +
> +		offset = 0;
> +		memcpy(dev + offset, PREFIX_ALLOW, allow_size);
> +		offset += allow_size;
> +		memcpy(dev + offset, devargs->name, name_length);
> +		offset += name_length;
> +		if (data_length > 0) {
> +			memcpy(dev + offset, devargs->data, data_length);

Do we need a comma "," before devargs->data in the output string?

> +			offset += data_length;
> +		}
> +		memset(dev + offset, 0x00, 1);
> +
> +		*(argv + count) = dev;
> +		count++;
> +	}
> +

Sorry for the late feedback, but I believe this whole block can be
hugely simplified by using asprintf.

	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
		if (devargs->data == NULL) {
			if (asprintf(&argv[count], PREFIX_ALLOW"%s", devargs->name) < 0)
				break;
		} else {
			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
					 devargs->name, devargs->data) < 0)
				break;
		}
		if (++count == max_capacity)
			break;
	}

This way, the only variables you actually need are devargs and 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 +97,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;
> +	int allow_num;
> +	int argv_num;
>  	int i, status;
>  	char path[32];
>  #ifdef RTE_LIB_PDUMP
> @@ -58,12 +113,17 @@ 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 + 1;
> +		argv_cpy = malloc(argv_num * sizeof(char *));

Check return from malloc.

>  		/* 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);
> +		if (num != allow_num)
> +			rte_panic("Fill allow parameter incomplete\n");
> +		num += numargs;
> +		argv_cpy[argv_num - 1] = NULL;
>  #ifdef RTE_EXEC_ENV_LINUX
>  		{
>  			const char *procdir = "/proc/" self "/fd/";
> @@ -131,6 +191,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 is ok to call with NULl, so you can skip the check and just do

	for (i = 0; i < num; i++)
		free(argv_cpy[i]);

> +		}
> +		free(argv_cpy);
>  	}
>  	/* parent process does a wait */
>  #ifdef RTE_LIB_PDUMP
> -- 
> 2.25.1
> 

[PATCH v6] app/test: secondary process passes allow parameters

[Mingjin Ye]

In EAL related test cases, the allow parameters are not passed to
the secondary process, resulting in unexpected NICs being loaded.

This patch fixes this issue by appending the allow parameters to
the secondary process.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v5: Optimized.
---
v6: Optimized.
---
 app/test/process.h | 52 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index af7bc3e0de..cd3603b7bb 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,34 @@ 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;
+
+	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
+		if (strlen(devargs->name) == 0)
+			continue;
+
+		if (strlen(devargs->data) == 0) {
+			if (asprintf(&argv[count], PREFIX_ALLOW"%s", devargs->name) < 0)
+				break;
+		} else {
+			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
+					 devargs->name, devargs->data) < 0)
+				break;
+		}
+
+		if (++count == max_capacity)
+			break;
+	}
+
+	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 +74,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;
+	int allow_num;
+	int argv_num;
 	int i, status;
 	char path[32];
 #ifdef RTE_LIB_PDUMP
@@ -58,12 +90,21 @@ 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 + 1;
+		argv_cpy = malloc(argv_num * sizeof(char *));
+		if (!argv_cpy)
+			rte_panic("Memory allocation failed\n");
+
 		/* 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);
+		if (num != allow_num)
+			rte_panic("Fill allow parameter incomplete\n");
 
+		num += numargs;
+		argv_cpy[argv_num - 1] = NULL;
 #ifdef RTE_EXEC_ENV_LINUX
 		{
 			const char *procdir = "/proc/" self "/fd/";
@@ -131,6 +172,11 @@ 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++)
+			free(argv_cpy[i]);
+
+		free(argv_cpy);
 	}
 	/* parent process does a wait */
 #ifdef RTE_LIB_PDUMP
-- 
2.25.1

Re: [PATCH v6] app/test: secondary process passes allow parameters

[Bruce Richardson]

On Mon, Nov 13, 2023 at 10:42:22AM +0000, Mingjin Ye wrote:
> In EAL related test cases, the allow parameters are not passed to
> the secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to
> the secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

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

> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
>  app/test/process.h | 52 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h
> index af7bc3e0de..cd3603b7bb 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,34 @@ 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;
> +
> +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> +		if (strlen(devargs->name) == 0)
> +			continue;
> +
> +		if (strlen(devargs->data) == 0) {
> +			if (asprintf(&argv[count], PREFIX_ALLOW"%s", devargs->name) < 0)
> +				break;
> +		} else {
> +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> +					 devargs->name, devargs->data) < 0)
> +				break;
> +		}
> +
> +		if (++count == max_capacity)
> +			break;
> +	}
> +
> +	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 +74,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;
> +	int allow_num;
> +	int argv_num;
>  	int i, status;
>  	char path[32];
>  #ifdef RTE_LIB_PDUMP
> @@ -58,12 +90,21 @@ 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 + 1;
> +		argv_cpy = malloc(argv_num * sizeof(char *));
> +		if (!argv_cpy)
> +			rte_panic("Memory allocation failed\n");
> +
>  		/* 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);
> +		if (num != allow_num)
> +			rte_panic("Fill allow parameter incomplete\n");
>  
> +		num += numargs;
> +		argv_cpy[argv_num - 1] = NULL;
>  #ifdef RTE_EXEC_ENV_LINUX
>  		{
>  			const char *procdir = "/proc/" self "/fd/";
> @@ -131,6 +172,11 @@ 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++)
> +			free(argv_cpy[i]);
> +
> +		free(argv_cpy);
>  	}
>  	/* parent process does a wait */
>  #ifdef RTE_LIB_PDUMP
> -- 
> 2.25.1
> 

[PATCH v7] app/test: secondary process passes allow parameters

[Mingjin Ye]

In EAL related test cases, the allow parameters are not passed to
the secondary process, resulting in unexpected NICs being loaded.

This patch fixes this issue by appending the allow parameters to
the secondary process.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v5: Optimized.
---
v6: Optimized.
---
v7: Fix CI errors.
---
 app/test/process.h | 46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index af7bc3e0de..06b2f8b192 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -17,6 +17,7 @@
 #include <dirent.h>
 
 #include <rte_string_fns.h> /* strlcpy */
+#include <rte_devargs.h>
 
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
@@ -34,6 +35,34 @@ 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;
+
+	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
+		if (strlen(devargs->name) == 0)
+			continue;
+
+		if (devargs->data == NULL || strlen(devargs->data) == 0) {
+			if (asprintf(&argv[count], PREFIX_ALLOW"%s", devargs->name) < 0)
+				break;
+		} else {
+			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
+					 devargs->name, devargs->data) < 0)
+				break;
+		}
+
+		if (++count == max_capacity)
+			break;
+	}
+
+	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
@@ -43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;
 static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
-	int num;
-	char *argv_cpy[numargs + 1];
+	int num = 0;
+	char **argv_cpy;
+	int allow_num;
+	int argv_num;
 	int i, status;
 	char path[32];
 #ifdef RTE_LIB_PDUMP
@@ -58,11 +89,18 @@ 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 + 1;
+		argv_cpy = calloc(argv_num, sizeof(char *));
+		if (!argv_cpy)
+			rte_panic("Memory allocation failed\n");
+
 		/* 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;
+		if (allow_num > 0)
+			num = add_parameter_allow(&argv_cpy[i], allow_num);
+		num += numargs;
 
 #ifdef RTE_EXEC_ENV_LINUX
 		{
-- 
2.25.1

RE: [PATCH v7] app/test: secondary process passes allow parameters

[Huang, ZhiminX]

> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Tuesday, November 14, 2023 6:28 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> <mingjinx.ye@intel.com>; stable@dpdk.org
> Subject: [PATCH v7] app/test: secondary process passes allow parameters
>
> In EAL related test cases, the allow parameters are not passed to the
> secondary process, resulting in unexpected NICs being loaded.
>
> This patch fixes this issue by appending the allow parameters to the secondary
> process.
>
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
>
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
> v7: Fix CI errors.
> ---

Verified pass for meson test:
dpdk-devbind.py -b vfio-pci 31:00.0 31:00.1 b1:00.0 b1:00.1
DPDK_TEST=eal_flags_file_prefix_autotest MALLOC_PERTURB_=192 /root/dpdk/x86_64-native-linuxapp-gcc/app/test/dpdk-test --file-prefix=eal_flags_file_prefix_autotest -a 31:00.0 -a 31:00.1

Tested-by: Zhimin Huang <zhiminx.huang@intel.com >

RE: [PATCH v7] app/test: secondary process passes allow parameters

[Ye, MingjinX]

Hi Richardson,

can you please take a look at this patch.

Thanks,
Mingjin


> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Tuesday, November 14, 2023 6:28 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> <mingjinx.ye@intel.com>; stable@dpdk.org
> Subject: [PATCH v7] app/test: secondary process passes allow parameters
> 
> In EAL related test cases, the allow parameters are not passed to the
> secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to the
> secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
> v7: Fix CI errors.
> ---
>  app/test/process.h | 46
> ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h index
> af7bc3e0de..06b2f8b192 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -17,6 +17,7 @@
>  #include <dirent.h>
> 
>  #include <rte_string_fns.h> /* strlcpy */
> +#include <rte_devargs.h>
> 
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  #define self "curproc"
> @@ -34,6 +35,34 @@ 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;
> +
> +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> +		if (strlen(devargs->name) == 0)
> +			continue;
> +
> +		if (devargs->data == NULL || strlen(devargs->data) == 0) {
> +			if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> devargs->name) < 0)
> +				break;
> +		} else {
> +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> +					 devargs->name, devargs->data) < 0)
> +				break;
> +		}
> +
> +		if (++count == max_capacity)
> +			break;
> +	}
> +
> +	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 @@ -
> 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline int
> process_dup(const char *const argv[], int numargs, const char *env_value)  {
> -	int num;
> -	char *argv_cpy[numargs + 1];
> +	int num = 0;
> +	char **argv_cpy;
> +	int allow_num;
> +	int argv_num;
>  	int i, status;
>  	char path[32];
>  #ifdef RTE_LIB_PDUMP
> @@ -58,11 +89,18 @@ 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 + 1;
> +		argv_cpy = calloc(argv_num, sizeof(char *));
> +		if (!argv_cpy)
> +			rte_panic("Memory allocation failed\n");
> +
>  		/* 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;
> +		if (allow_num > 0)
> +			num = add_parameter_allow(&argv_cpy[i],
> allow_num);
> +		num += numargs;
> 
>  #ifdef RTE_EXEC_ENV_LINUX
>  		{
> --
> 2.25.1

Re: [PATCH v7] app/test: secondary process passes allow parameters

[Bruce Richardson]

On Fri, Nov 17, 2023 at 10:24:41AM +0000, Ye, MingjinX wrote:
> Hi Richardson,
> 
> can you please take a look at this patch.
> 
> Thanks,
> Mingjin
> 

Hi,

I acked v6. If nothing major has changed in the patch, then the ack can be
kept for v7. If there is something that has changed that you think I should
look at, please include it in the change-log, so I can check it out.

For now, assuming no major changes:

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

/Bruce

> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Tuesday, November 14, 2023 6:28 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; stable@dpdk.org
> > Subject: [PATCH v7] app/test: secondary process passes allow parameters
> > 
> > In EAL related test cases, the allow parameters are not passed to the
> > secondary process, resulting in unexpected NICs being loaded.
> > 
> > This patch fixes this issue by appending the allow parameters to the
> > secondary process.
> > 
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> > v5: Optimized.
> > ---
> > v6: Optimized.
> > ---
> > v7: Fix CI errors.
> > ---
> >  app/test/process.h | 46
> > ++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 42 insertions(+), 4 deletions(-)
> > 
> > diff --git a/app/test/process.h b/app/test/process.h index
> > af7bc3e0de..06b2f8b192 100644
> > --- a/app/test/process.h
> > +++ b/app/test/process.h
> > @@ -17,6 +17,7 @@
> >  #include <dirent.h>
> > 
> >  #include <rte_string_fns.h> /* strlcpy */
> > +#include <rte_devargs.h>
> > 
> >  #ifdef RTE_EXEC_ENV_FREEBSD
> >  #define self "curproc"
> > @@ -34,6 +35,34 @@ 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;
> > +
> > +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> > +		if (strlen(devargs->name) == 0)
> > +			continue;
> > +
> > +		if (devargs->data == NULL || strlen(devargs->data) == 0) {
> > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> > devargs->name) < 0)
> > +				break;
> > +		} else {
> > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> > +					 devargs->name, devargs->data) < 0)
> > +				break;
> > +		}
> > +
> > +		if (++count == max_capacity)
> > +			break;
> > +	}
> > +
> > +	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 @@ -
> > 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline int
> > process_dup(const char *const argv[], int numargs, const char *env_value)  {
> > -	int num;
> > -	char *argv_cpy[numargs + 1];
> > +	int num = 0;
> > +	char **argv_cpy;
> > +	int allow_num;
> > +	int argv_num;
> >  	int i, status;
> >  	char path[32];
> >  #ifdef RTE_LIB_PDUMP
> > @@ -58,11 +89,18 @@ 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 + 1;
> > +		argv_cpy = calloc(argv_num, sizeof(char *));
> > +		if (!argv_cpy)
> > +			rte_panic("Memory allocation failed\n");
> > +
> >  		/* 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;
> > +		if (allow_num > 0)
> > +			num = add_parameter_allow(&argv_cpy[i],
> > allow_num);
> > +		num += numargs;
> > 
> >  #ifdef RTE_EXEC_ENV_LINUX
> >  		{
> > --
> > 2.25.1
> 

RE: [PATCH v7] app/test: secondary process passes allow parameters

[Ye, MingjinX]

Hi, 

Thanks for the ack. would like to change the state of the patch, which will be merged into the dpdk.

Change: The "--allow" parameter is added when the number of allow is greater than zero.

Thanks,
Mingjin

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Friday, November 17, 2023 7:17 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>;
> stable@dpdk.org; Xu, HailinX <hailinx.xu@intel.com>
> Subject: Re: [PATCH v7] app/test: secondary process passes allow
> parameters
> 
> On Fri, Nov 17, 2023 at 10:24:41AM +0000, Ye, MingjinX wrote:
> > Hi Richardson,
> >
> > can you please take a look at this patch.
> >
> > Thanks,
> > Mingjin
> >
> 
> Hi,
> 
> I acked v6. If nothing major has changed in the patch, then the ack can be
> kept for v7. If there is something that has changed that you think I should
> look at, please include it in the change-log, so I can check it out.
> 
> For now, assuming no major changes:
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> /Bruce
> 
> >
> > > -----Original Message-----
> > > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > > Sent: Tuesday, November 14, 2023 6:28 PM
> > > To: dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> > > <mingjinx.ye@intel.com>; stable@dpdk.org
> > > Subject: [PATCH v7] app/test: secondary process passes allow
> > > parameters
> > >
> > > In EAL related test cases, the allow parameters are not passed to
> > > the secondary process, resulting in unexpected NICs being loaded.
> > >
> > > This patch fixes this issue by appending the allow parameters to the
> > > secondary process.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > > ---
> > > v5: Optimized.
> > > ---
> > > v6: Optimized.
> > > ---
> > > v7: Fix CI errors.
> > > ---
> > >  app/test/process.h | 46
> > > ++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 42 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/app/test/process.h b/app/test/process.h index
> > > af7bc3e0de..06b2f8b192 100644
> > > --- a/app/test/process.h
> > > +++ b/app/test/process.h
> > > @@ -17,6 +17,7 @@
> > >  #include <dirent.h>
> > >
> > >  #include <rte_string_fns.h> /* strlcpy */
> > > +#include <rte_devargs.h>
> > >
> > >  #ifdef RTE_EXEC_ENV_FREEBSD
> > >  #define self "curproc"
> > > @@ -34,6 +35,34 @@ 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;
> > > +
> > > +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> > > +		if (strlen(devargs->name) == 0)
> > > +			continue;
> > > +
> > > +		if (devargs->data == NULL || strlen(devargs->data) == 0) {
> > > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> > > devargs->name) < 0)
> > > +				break;
> > > +		} else {
> > > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> > > +					 devargs->name, devargs->data) < 0)
> > > +				break;
> > > +		}
> > > +
> > > +		if (++count == max_capacity)
> > > +			break;
> > > +	}
> > > +
> > > +	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 @@ -
> > > 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline
> > > int process_dup(const char *const argv[], int numargs, const char
> *env_value)  {
> > > -	int num;
> > > -	char *argv_cpy[numargs + 1];
> > > +	int num = 0;
> > > +	char **argv_cpy;
> > > +	int allow_num;
> > > +	int argv_num;
> > >  	int i, status;
> > >  	char path[32];
> > >  #ifdef RTE_LIB_PDUMP
> > > @@ -58,11 +89,18 @@ 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 + 1;
> > > +		argv_cpy = calloc(argv_num, sizeof(char *));
> > > +		if (!argv_cpy)
> > > +			rte_panic("Memory allocation failed\n");
> > > +
> > >  		/* 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;
> > > +		if (allow_num > 0)
> > > +			num = add_parameter_allow(&argv_cpy[i],
> > > allow_num);
> > > +		num += numargs;
> > >
> > >  #ifdef RTE_EXEC_ENV_LINUX
> > >  		{
> > > --
> > > 2.25.1
> >

Re: [PATCH v7] app/test: secondary process passes allow parameters

[Thomas Monjalon]

14/11/2023 11:28, Mingjin Ye:
> In EAL related test cases, the allow parameters are not passed to
> the secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to
> the secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Applied, thanks.

Re: [PATCH v2] app/test:subprocess synchronization of parameters

[Stephen Hemminger]

On Wed, 26 Jul 2023 10:39:47 +0800
Kaisen You <kaisenx.you@intel.com> wrote:

> In meson_test, because the child process does not synchronize
> the NIC startup parameters of the parent process at startup,
> it uses all NICs bound by vfio as startup parameters by default,
> and an exception occurs in the subsequent hugefile check,
> causing the test to fail. Synchronize the NIC startup parameters
> of the parent process to the child process to solve this problem.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>

It would to give direct example of test that fails i.e what
is the test command line.

This patch never got applied because of large number
of CI test failures, please fix and resubmit

Re: [PATCH] child process synchronization NIC startup parameters

[Stephen Hemminger]

On Wed,  5 Jul 2023 17:35:14 +0800
Kaisen You <kaisenx.you@intel.com> wrote:

>  
> +/* get cmdline form PID. Read process info form /proc/$PID. */
> +static char *get_cmdline_from_pid(pid_t pid, char *buf, int len)
> +{
> +	char filename[PATH_MAX];
> +	char *name = NULL;
> +	int fd;
> +	int ret;
> +
> +	if (pid < 1 || buf == NULL || len < 0) {
> +		printf("%s: illegal param\n", __func__);
> +		return NULL;
> +	}
> +
> +	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
> +	memset(buf, 0, len);
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0) {
> +		perror("open:");
> +		return NULL;
> +	}
> +	ret = read(fd, buf, len);
> +	close(fd);
> +
> +	if (ret < 0)
> +		return NULL;
> +
> +	if (buf[ret-1] == '\n')
> +		buf[--ret] = 0;
> +
> +	name = buf;
> +	while (ret) {
> +		if (((unsigned char)*name) < ' ')
> +			*name = ' ';
> +		name++;
> +		ret--;
> +	}
> +	*name = 0;
> +
> +	if (buf[0])
> +		return buf;
> +
> +	return NULL;
> +}

Bad idea, this is not portable, and easily broken by system changes and container
environments. Please use some form of explicit communication like is done
for primary / secondary process support.