patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH] child process synchronization NIC startup parameters
@ 2023-07-05  9:35 Kaisen You
  2023-07-06  2:16 ` Yang, Qiming
                   ` (5 more replies)
  0 siblings, 6 replies; 27+ messages in thread
From: Kaisen You @ 2023-07-05  9:35 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Kaisen You, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH] child process synchronization NIC startup parameters
  2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
@ 2023-07-06  2:16 ` Yang, Qiming
  2023-07-07  1:21   ` You, KaisenX
  2023-07-06 19:07 ` Stephen Hemminger
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 27+ messages in thread
From: Yang, Qiming @ 2023-07-06  2:16 UTC (permalink / raw)
  To: You, KaisenX, dev; +Cc: Zhou, YidingX, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] child process synchronization NIC startup parameters
  2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
  2023-07-06  2:16 ` Yang, Qiming
@ 2023-07-06 19:07 ` Stephen Hemminger
  2023-07-14  5:59 ` [PATCH v2] app/test:subprocess synchronization of parameters Kaisen You
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 27+ messages in thread
From: Stephen Hemminger @ 2023-07-06 19:07 UTC (permalink / raw)
  To: Kaisen You; +Cc: dev, qiming.yang, yidingx.zhou, stable

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?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH] child process synchronization NIC startup parameters
  2023-07-06  2:16 ` Yang, Qiming
@ 2023-07-07  1:21   ` You, KaisenX
  0 siblings, 0 replies; 27+ messages in thread
From: You, KaisenX @ 2023-07-07  1:21 UTC (permalink / raw)
  To: Yang, Qiming, dev; +Cc: Zhou, YidingX, stable



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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2] app/test:subprocess synchronization of parameters
  2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
  2023-07-06  2:16 ` Yang, Qiming
  2023-07-06 19:07 ` Stephen Hemminger
@ 2023-07-14  5:59 ` Kaisen You
  2023-07-17  1:47 ` Kaisen You
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 27+ messages in thread
From: Kaisen You @ 2023-07-14  5:59 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Kaisen You, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2] app/test:subprocess synchronization of parameters
  2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
                   ` (2 preceding siblings ...)
  2023-07-14  5:59 ` [PATCH v2] app/test:subprocess synchronization of parameters Kaisen You
@ 2023-07-17  1:47 ` Kaisen You
  2023-07-26  2:39 ` Kaisen You
  2024-06-13 23:23 ` [PATCH] child process synchronization NIC startup parameters Stephen Hemminger
  5 siblings, 0 replies; 27+ messages in thread
From: Kaisen You @ 2023-07-17  1:47 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Kaisen You, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2] app/test:subprocess synchronization of parameters
  2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
                   ` (3 preceding siblings ...)
  2023-07-17  1:47 ` Kaisen You
@ 2023-07-26  2:39 ` Kaisen You
  2023-09-25  9:42   ` [PATCH v3] app/test: secondary process passes allow parameters Mingjin Ye
  2024-10-03 16:00   ` [PATCH v2] app/test:subprocess synchronization of parameters Stephen Hemminger
  2024-06-13 23:23 ` [PATCH] child process synchronization NIC startup parameters Stephen Hemminger
  5 siblings, 2 replies; 27+ messages in thread
From: Kaisen You @ 2023-07-26  2:39 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Kaisen You, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v3] app/test: secondary process passes allow parameters
  2023-07-26  2:39 ` Kaisen You
@ 2023-09-25  9:42   ` Mingjin Ye
  2023-09-25  9:58     ` David Marchand
  2023-09-27  3:42     ` [PATCH v4] app/test: append 'allow' parameters to secondary processes Mingjin Ye
  2024-10-03 16:00   ` [PATCH v2] app/test:subprocess synchronization of parameters Stephen Hemminger
  1 sibling, 2 replies; 27+ messages in thread
From: Mingjin Ye @ 2023-09-25  9:42 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Mingjin Ye, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3] app/test: secondary process passes allow parameters
  2023-09-25  9:42   ` [PATCH v3] app/test: secondary process passes allow parameters Mingjin Ye
@ 2023-09-25  9:58     ` David Marchand
  2023-09-25 10:09       ` Ye, MingjinX
  2023-09-27  3:42     ` [PATCH v4] app/test: append 'allow' parameters to secondary processes Mingjin Ye
  1 sibling, 1 reply; 27+ messages in thread
From: David Marchand @ 2023-09-25  9:58 UTC (permalink / raw)
  To: Mingjin Ye; +Cc: dev, qiming.yang, yidingx.zhou, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH v3] app/test: secondary process passes allow parameters
  2023-09-25  9:58     ` David Marchand
@ 2023-09-25 10:09       ` Ye, MingjinX
  2023-09-25 10:19         ` David Marchand
  0 siblings, 1 reply; 27+ messages in thread
From: Ye, MingjinX @ 2023-09-25 10:09 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Yang, Qiming, Zhou, YidingX, stable



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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3] app/test: secondary process passes allow parameters
  2023-09-25 10:09       ` Ye, MingjinX
@ 2023-09-25 10:19         ` David Marchand
  0 siblings, 0 replies; 27+ messages in thread
From: David Marchand @ 2023-09-25 10:19 UTC (permalink / raw)
  To: Ye, MingjinX; +Cc: dev, Yang, Qiming, Zhou, YidingX, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v4] app/test: append 'allow' parameters to secondary processes
  2023-09-25  9:42   ` [PATCH v3] app/test: secondary process passes allow parameters Mingjin Ye
  2023-09-25  9:58     ` David Marchand
@ 2023-09-27  3:42     ` Mingjin Ye
  2023-10-13  2:47       ` Huang, ZhiminX
                         ` (2 more replies)
  1 sibling, 3 replies; 27+ messages in thread
From: Mingjin Ye @ 2023-09-27  3:42 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Mingjin Ye, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH v4] app/test: append 'allow' parameters to secondary processes
  2023-09-27  3:42     ` [PATCH v4] app/test: append 'allow' parameters to secondary processes Mingjin Ye
@ 2023-10-13  2:47       ` Huang, ZhiminX
  2023-11-09 11:13       ` Bruce Richardson
  2023-11-10 10:30       ` [PATCH v5] app/test: secondary process passes allow parameters Mingjin Ye
  2 siblings, 0 replies; 27+ messages in thread
From: Huang, ZhiminX @ 2023-10-13  2:47 UTC (permalink / raw)
  To: Ye, MingjinX, dev; +Cc: Yang, Qiming, Zhou, YidingX, Ye, MingjinX, stable

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



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4] app/test: append 'allow' parameters to secondary processes
  2023-09-27  3:42     ` [PATCH v4] app/test: append 'allow' parameters to secondary processes Mingjin Ye
  2023-10-13  2:47       ` Huang, ZhiminX
@ 2023-11-09 11:13       ` Bruce Richardson
  2023-11-10 10:30       ` [PATCH v5] app/test: secondary process passes allow parameters Mingjin Ye
  2 siblings, 0 replies; 27+ messages in thread
From: Bruce Richardson @ 2023-11-09 11:13 UTC (permalink / raw)
  To: Mingjin Ye; +Cc: dev, qiming.yang, yidingx.zhou, stable

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
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5] app/test: secondary process passes allow parameters
  2023-09-27  3:42     ` [PATCH v4] app/test: append 'allow' parameters to secondary processes Mingjin Ye
  2023-10-13  2:47       ` Huang, ZhiminX
  2023-11-09 11:13       ` Bruce Richardson
@ 2023-11-10 10:30       ` Mingjin Ye
  2023-11-10 11:18         ` Bruce Richardson
  2023-11-13 10:42         ` [PATCH v6] " Mingjin Ye
  2 siblings, 2 replies; 27+ messages in thread
From: Mingjin Ye @ 2023-11-10 10:30 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, Mingjin Ye, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v5] app/test: secondary process passes allow parameters
  2023-11-10 10:30       ` [PATCH v5] app/test: secondary process passes allow parameters Mingjin Ye
@ 2023-11-10 11:18         ` Bruce Richardson
  2023-11-13 10:42         ` [PATCH v6] " Mingjin Ye
  1 sibling, 0 replies; 27+ messages in thread
From: Bruce Richardson @ 2023-11-10 11:18 UTC (permalink / raw)
  To: Mingjin Ye; +Cc: dev, qiming.yang, stable

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
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v6] app/test: secondary process passes allow parameters
  2023-11-10 10:30       ` [PATCH v5] app/test: secondary process passes allow parameters Mingjin Ye
  2023-11-10 11:18         ` Bruce Richardson
@ 2023-11-13 10:42         ` Mingjin Ye
  2023-11-13 11:11           ` Bruce Richardson
  2023-11-14 10:28           ` [PATCH v7] " Mingjin Ye
  1 sibling, 2 replies; 27+ messages in thread
From: Mingjin Ye @ 2023-11-13 10:42 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, Mingjin Ye, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v6] app/test: secondary process passes allow parameters
  2023-11-13 10:42         ` [PATCH v6] " Mingjin Ye
@ 2023-11-13 11:11           ` Bruce Richardson
  2023-11-14 10:28           ` [PATCH v7] " Mingjin Ye
  1 sibling, 0 replies; 27+ messages in thread
From: Bruce Richardson @ 2023-11-13 11:11 UTC (permalink / raw)
  To: Mingjin Ye; +Cc: dev, qiming.yang, stable

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
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7] app/test: secondary process passes allow parameters
  2023-11-13 10:42         ` [PATCH v6] " Mingjin Ye
  2023-11-13 11:11           ` Bruce Richardson
@ 2023-11-14 10:28           ` Mingjin Ye
  2023-11-17 10:05             ` Huang, ZhiminX
                               ` (2 more replies)
  1 sibling, 3 replies; 27+ messages in thread
From: Mingjin Ye @ 2023-11-14 10:28 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, Mingjin Ye, stable

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH v7] app/test: secondary process passes allow parameters
  2023-11-14 10:28           ` [PATCH v7] " Mingjin Ye
@ 2023-11-17 10:05             ` Huang, ZhiminX
  2023-11-17 10:24             ` Ye, MingjinX
  2024-03-06 13:50             ` Thomas Monjalon
  2 siblings, 0 replies; 27+ messages in thread
From: Huang, ZhiminX @ 2023-11-17 10:05 UTC (permalink / raw)
  To: Ye, MingjinX, dev; +Cc: Yang, Qiming, Ye, MingjinX, stable



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



^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH v7] app/test: secondary process passes allow parameters
  2023-11-14 10:28           ` [PATCH v7] " Mingjin Ye
  2023-11-17 10:05             ` Huang, ZhiminX
@ 2023-11-17 10:24             ` Ye, MingjinX
  2023-11-17 11:17               ` Bruce Richardson
  2024-03-06 13:50             ` Thomas Monjalon
  2 siblings, 1 reply; 27+ messages in thread
From: Ye, MingjinX @ 2023-11-17 10:24 UTC (permalink / raw)
  To: dev, Richardson, Bruce; +Cc: Yang, Qiming, stable, Xu, HailinX

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7] app/test: secondary process passes allow parameters
  2023-11-17 10:24             ` Ye, MingjinX
@ 2023-11-17 11:17               ` Bruce Richardson
  2023-11-24 10:28                 ` Ye, MingjinX
  0 siblings, 1 reply; 27+ messages in thread
From: Bruce Richardson @ 2023-11-17 11:17 UTC (permalink / raw)
  To: Ye, MingjinX; +Cc: dev, Yang, Qiming, stable, Xu, HailinX

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
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH v7] app/test: secondary process passes allow parameters
  2023-11-17 11:17               ` Bruce Richardson
@ 2023-11-24 10:28                 ` Ye, MingjinX
  0 siblings, 0 replies; 27+ messages in thread
From: Ye, MingjinX @ 2023-11-24 10:28 UTC (permalink / raw)
  To: Richardson, Bruce; +Cc: dev, Yang, Qiming, stable, Xu, HailinX

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7] app/test: secondary process passes allow parameters
  2023-11-14 10:28           ` [PATCH v7] " Mingjin Ye
  2023-11-17 10:05             ` Huang, ZhiminX
  2023-11-17 10:24             ` Ye, MingjinX
@ 2024-03-06 13:50             ` Thomas Monjalon
  2 siblings, 0 replies; 27+ messages in thread
From: Thomas Monjalon @ 2024-03-06 13:50 UTC (permalink / raw)
  To: Mingjin Ye; +Cc: dev, qiming.yang, stable

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.




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] child process synchronization NIC startup parameters
  2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
                   ` (4 preceding siblings ...)
  2023-07-26  2:39 ` Kaisen You
@ 2024-06-13 23:23 ` Stephen Hemminger
  5 siblings, 0 replies; 27+ messages in thread
From: Stephen Hemminger @ 2024-06-13 23:23 UTC (permalink / raw)
  To: Kaisen You; +Cc: dev, qiming.yang, yidingx.zhou, stable

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.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] app/test:subprocess synchronization of parameters
  2023-07-26  2:39 ` Kaisen You
  2023-09-25  9:42   ` [PATCH v3] app/test: secondary process passes allow parameters Mingjin Ye
@ 2024-10-03 16:00   ` Stephen Hemminger
  1 sibling, 0 replies; 27+ messages in thread
From: Stephen Hemminger @ 2024-10-03 16:00 UTC (permalink / raw)
  To: Kaisen You; +Cc: dev, qiming.yang, yidingx.zhou, stable

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH] child process synchronization NIC startup parameters
@ 2023-07-05  7:43 Kaisen You
  0 siblings, 0 replies; 27+ messages in thread
From: Kaisen You @ 2023-07-05  7:43 UTC (permalink / raw)
  To: dev; +Cc: qiming.yang, yidingx.zhou, Kaisen You, stable

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 | 81 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 3 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..95dffab69c 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -15,6 +15,8 @@
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
+#include <fcntl.h>
+#include <ctype.h>
 
 #include <rte_string_fns.h> /* strlcpy */
 
@@ -24,6 +26,7 @@
 #else
 #define self "self"
 #define exe "exe"
+#define MAX_EXTRA_ARGS 32
 #endif
 
 #ifdef RTE_LIB_PDUMP
@@ -34,6 +37,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 +91,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 +166,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2024-10-03 16:00 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-05  9:35 [PATCH] child process synchronization NIC startup parameters Kaisen You
2023-07-06  2:16 ` Yang, Qiming
2023-07-07  1:21   ` You, KaisenX
2023-07-06 19:07 ` Stephen Hemminger
2023-07-14  5:59 ` [PATCH v2] app/test:subprocess synchronization of parameters Kaisen You
2023-07-17  1:47 ` Kaisen You
2023-07-26  2:39 ` Kaisen You
2023-09-25  9:42   ` [PATCH v3] app/test: secondary process passes allow parameters Mingjin Ye
2023-09-25  9:58     ` David Marchand
2023-09-25 10:09       ` Ye, MingjinX
2023-09-25 10:19         ` David Marchand
2023-09-27  3:42     ` [PATCH v4] app/test: append 'allow' parameters to secondary processes Mingjin Ye
2023-10-13  2:47       ` Huang, ZhiminX
2023-11-09 11:13       ` Bruce Richardson
2023-11-10 10:30       ` [PATCH v5] app/test: secondary process passes allow parameters Mingjin Ye
2023-11-10 11:18         ` Bruce Richardson
2023-11-13 10:42         ` [PATCH v6] " Mingjin Ye
2023-11-13 11:11           ` Bruce Richardson
2023-11-14 10:28           ` [PATCH v7] " Mingjin Ye
2023-11-17 10:05             ` Huang, ZhiminX
2023-11-17 10:24             ` Ye, MingjinX
2023-11-17 11:17               ` Bruce Richardson
2023-11-24 10:28                 ` Ye, MingjinX
2024-03-06 13:50             ` Thomas Monjalon
2024-10-03 16:00   ` [PATCH v2] app/test:subprocess synchronization of parameters Stephen Hemminger
2024-06-13 23:23 ` [PATCH] child process synchronization NIC startup parameters Stephen Hemminger
  -- strict thread matches above, loose matches on Subject: below --
2023-07-05  7:43 Kaisen You

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).