RE: [PATCH] SCSY-192443 Fix DPDK-dumpcap for XRAN Library

RE: [PATCH] SCSY-192443 Fix DPDK-dumpcap for XRAN Library

[Pattan, Reshma]

> -----Original Message-----
> From: Kaur, Arshdeep <arshdeep.kaur@intel.com>
> Subject: [PATCH] SCSY-192443 Fix DPDK-dumpcap for XRAN Library
>  <snip>

> Issue: By default, dpdk-dumpcap tries to listen to
> /var/run/dpdk/rte/mp_socket, whereas in flexran a socket is created at
> /var/run/dpdk/<file-prefix>/mp_socket. File prefix is provided to flexran via
> config files which is used in EAL options. There is no way in dpdk-dumpcap
> today to provide file-prefix.
> 
> Fix: Added a new parameter "-m" to handle this requirement. User needs to
> provide "-m <file-prefix> as first argument to dpdk-dumpcap.



Remove SCSY* from heading

Heading should beging with the example name that you are modifying  that is "dumpcap:".

Tell exactly what you are adding in this patch. No need to add  for whom you are doing this patch and all.

So, Heading can be as simple as "dumpcap:  add the mutiprocess fileprefix support"

With these changes you can send the V2.

This is not the issue and you are not fixing it. This is some new support you are adding.  So need to reframe your commit message.

Thanks,
Reshma

RE: [PATCH v2] dumpcap: add the mutiprocess fileprefix support.

[Kaur, Arshdeep]

> -----Original Message-----
> From: Kaur, Arshdeep <arshdeep.kaur@intel.com>
> Sent: Wednesday, September 7, 2022 9:57 PM
> To: dev@dpdk.org
> Cc: Kaur, Arshdeep <arshdeep.kaur@intel.com>
> Subject: [PATCH v2] dumpcap: add the mutiprocess fileprefix support.

Gentle reminder for review.

[PATCH v2] dumpcap: add the mutiprocess fileprefix support.

[Arshdeep Kaur]

New optional parameter "-m <fileprefix>" added.
It will update the mp_socket path for multiprocess communication.
Default : '/var/run/dpdk/rte/mp_socket'
Updated : '/var/run/dpdk/<file-prefix>/mp_socket'

Note: Give -m as first argument.

Signed-off-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
---
 app/dumpcap/main.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index 8972c45a71..1aa43ad9c1 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -323,7 +323,7 @@ static void parse_opts(int argc, char **argv)
 	int option_index, c;
 
 	for (;;) {
-		c = getopt_long(argc, argv, "a:b:c:dDf:ghi:nN:pPqs:vw:",
+		c = getopt_long(argc, argv, "a:b:c:dDf:ghi:nN:m:pPqs:vw:",
 				long_options, &option_index);
 		if (c == -1)
 			break;
@@ -392,6 +392,9 @@ static void parse_opts(int argc, char **argv)
 		case 'v':
 			printf("%s\n", version());
 			exit(0);
+		case 'm':
+			/* Handled before dpdk_init */
+			break;
 		default:
 			fprintf(stderr, "Invalid option: %s\n",
 				argv[optind - 1]);
@@ -507,10 +510,10 @@ report_packet_stats(dumpcap_out_t out)
  * typical EAL command line arguments.
  * We don't want to expose all the DPDK internals to the user.
  */
-static void dpdk_init(void)
+static void dpdk_init(const char * const prefix)
 {
-	static const char * const args[] = {
-		"dumpcap", "--proc-type", "secondary",
+		const char * const args[] = {
+		"dumpcap", "--proc-type", "secondary", "--file-prefix", prefix,
 		"--log-level", "notice"
 
 	};
@@ -787,7 +790,17 @@ int main(int argc, char **argv)
 	progname = argv[0];
 
 	parse_opts(argc, argv);
-	dpdk_init();
+	char prefix[256];
+	strcpy(prefix, "rte");
+
+	printf("\nIMP:: Please provide -m file_prefix as first argument in case non-default mp_socket path is to be setup (e.g. ./dpdk-dumpcap -m wls_0)\n\n");
+	/* In order to use mp_socket path other than default, give -m as first argument. */
+	if (argc >= 3) {
+		if (strncmp(argv[1], "-m", 2) == 0)
+			strncpy(prefix, argv[2], sizeof(prefix));
+	}
+
+	dpdk_init(prefix);
 
 	if (filter_str)
 		compile_filter();
-- 
2.37.1

Re: [PATCH v2] dumpcap: add the mutiprocess fileprefix support.

[Stephen Hemminger]

On Mon, 12 Sep 2022 05:43:09 -0700
Arshdeep Kaur <arshdeep.kaur@intel.com> wrote:

> New optional parameter "-m <fileprefix>" added.
> It will update the mp_socket path for multiprocess communication.
> Default : '/var/run/dpdk/rte/mp_socket'
> Updated : '/var/run/dpdk/<file-prefix>/mp_socket'
> 
> Note: Give -m as first argument.
> 
> Signed-off-by: Arshdeep Kaur <arshdeep.kaur@intel.com>

The args to dumpcap are chosen to be the same as existing
wireshark dumpcap. This probably should be long option only.
Why not use an environment variable instead?

> ---
>  app/dumpcap/main.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index 8972c45a71..1aa43ad9c1 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -323,7 +323,7 @@ static void parse_opts(int argc, char **argv)
>  	int option_index, c;
>  
>  	for (;;) {
> -		c = getopt_long(argc, argv, "a:b:c:dDf:ghi:nN:pPqs:vw:",
> +		c = getopt_long(argc, argv, "a:b:c:dDf:ghi:nN:m:pPqs:vw:",
>  				long_options, &option_index);
>  		if (c == -1)
>  			break;
> @@ -392,6 +392,9 @@ static void parse_opts(int argc, char **argv)
>  		case 'v':
>  			printf("%s\n", version());
>  			exit(0);
> +		case 'm':
> +			/* Handled before dpdk_init */
> +			break;
>  		default:
>  			fprintf(stderr, "Invalid option: %s\n",
>  				argv[optind - 1]);
> @@ -507,10 +510,10 @@ report_packet_stats(dumpcap_out_t out)
>   * typical EAL command line arguments.
>   * We don't want to expose all the DPDK internals to the user.
>   */
> -static void dpdk_init(void)
> +static void dpdk_init(const char * const prefix)
>  {
> -	static const char * const args[] = {
> -		"dumpcap", "--proc-type", "secondary",
> +		const char * const args[] = {
> +		"dumpcap", "--proc-type", "secondary", "--file-prefix", prefix,
>  		"--log-level", "notice"
>  
>  	};
> @@ -787,7 +790,17 @@ int main(int argc, char **argv)
>  	progname = argv[0];
>  
>  	parse_opts(argc, argv);
> -	dpdk_init();
> +	char prefix[256];

Slightly short.
> +	strcpy(prefix, "rte");
> +
> +	printf("\nIMP:: Please provide -m file_prefix as first argument in case non-default mp_socket path is to be setup (e.g. ./dpdk-dumpcap -m wls_0)\n\n");

Error message should look like other code.

> +	/* In order to use mp_socket path other than default, give -m as first argument. */
> +	if (argc >= 3) {
> +		if (strncmp(argv[1], "-m", 2) == 0)
> +			strncpy(prefix, argv[2], sizeof(prefix));
> +	}
> +
> +	dpdk_init(prefix);
>  
>  	if (filter_str)
>  		compile_filter();

[RFT] dumpcap: add file-prefix option

[Stephen Hemminger]

When using dumpcap in container environment or with multiple
DPDK processes, it is useful to be able to specify file prefix.

This version only accepts the long format option used by
other commands. If no prefix is specified then the default
is used.

Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
Did basic command line test, but still needs testing with a prefix
being used (ie multiple apps).

 app/dumpcap/main.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
index a6041d4ff495..bdeef96d9c0b 100644
--- a/app/dumpcap/main.c
+++ b/app/dumpcap/main.c
@@ -61,6 +61,7 @@ static char *output_name;
 static const char *filter_str;
 static unsigned int ring_size = 2048;
 static const char *capture_comment;
+static const char *file_prefix;
 static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;
 static bool dump_bpf;
 static struct {
@@ -122,6 +123,7 @@ static void usage(void)
 	       "                           add a capture comment to the output file\n"
 	       "\n"
 	       "Miscellaneous:\n"
+	       "  --file-prefix=<prefix>   prefix to use for multi-process\n"
 	       "  -q                       don't report packet capture counts\n"
 	       "  -v, --version            print version information and exit\n"
 	       "  -h, --help               display this help and exit\n"
@@ -310,6 +312,7 @@ static void parse_opts(int argc, char **argv)
 	static const struct option long_options[] = {
 		{ "autostop",        required_argument, NULL, 'a' },
 		{ "capture-comment", required_argument, NULL, 0 },
+		{ "file-prefix",     required_argument, NULL, 0 },
 		{ "help",            no_argument,       NULL, 'h' },
 		{ "interface",       required_argument, NULL, 'i' },
 		{ "list-interfaces", no_argument,       NULL, 'D' },
@@ -330,11 +333,13 @@ static void parse_opts(int argc, char **argv)
 
 		switch (c) {
 		case 0:
-			switch (option_index) {
-			case 0:
+			if (!strcmp(long_options[option_index].name,
+				    "capture-comment")) {
 				capture_comment = optarg;
-				break;
-			default:
+			} else if (!strcmp(long_options[option_index].name,
+					   "file-prefix")) {
+				file_prefix = optarg;
+			} else {
 				usage();
 				exit(1);
 			}
@@ -512,12 +517,14 @@ static void dpdk_init(void)
 	static const char * const args[] = {
 		"dumpcap", "--proc-type", "secondary",
 		"--log-level", "notice"
-
 	};
-	const int eal_argc = RTE_DIM(args);
+	int eal_argc = RTE_DIM(args);
 	char **eal_argv;
 	unsigned int i;
 
+	if (file_prefix != NULL)
+		eal_argc += 2;
+
 	/* DPDK API requires mutable versions of command line arguments. */
 	eal_argv = calloc(eal_argc + 1, sizeof(char *));
 	if (eal_argv == NULL)
@@ -527,6 +534,11 @@ static void dpdk_init(void)
 	for (i = 1; i < RTE_DIM(args); i++)
 		eal_argv[i] = strdup(args[i]);
 
+	if (file_prefix != NULL) {
+		eal_argv[i++] = strdup("--file-prefix");
+		eal_argv[i++] = strdup(file_prefix);
+	}
+
 	if (rte_eal_init(eal_argc, eal_argv) < 0)
 		rte_exit(EXIT_FAILURE, "EAL init failed: is primary process running?\n");
 
-- 
2.35.1

RE: [RFT] dumpcap: add file-prefix option

[Kaur, Arshdeep]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, September 13, 2022 12:34 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur, Arshdeep
> <arshdeep.kaur@intel.com>
> Subject: [RFT] dumpcap: add file-prefix option
> 
> When using dumpcap in container environment or with multiple DPDK
> processes, it is useful to be able to specify file prefix.
> 
> This version only accepts the long format option used by other commands.
> If no prefix is specified then the default is used.
> 
> Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> Did basic command line test, but still needs testing with a prefix being used
> (ie multiple apps).
> 
>  app/dumpcap/main.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c index
> a6041d4ff495..bdeef96d9c0b 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -61,6 +61,7 @@ static char *output_name;  static const char
> *filter_str;  static unsigned int ring_size = 2048;  static const char
> *capture_comment;
> +static const char *file_prefix;
>  static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;  static bool
> dump_bpf;  static struct { @@ -122,6 +123,7 @@ static void usage(void)
>  	       "                           add a capture comment to the output file\n"
>  	       "\n"
>  	       "Miscellaneous:\n"
> +	       "  --file-prefix=<prefix>   prefix to use for multi-process\n"
>  	       "  -q                       don't report packet capture counts\n"
>  	       "  -v, --version            print version information and exit\n"
>  	       "  -h, --help               display this help and exit\n"
> @@ -310,6 +312,7 @@ static void parse_opts(int argc, char **argv)
>  	static const struct option long_options[] = {
>  		{ "autostop",        required_argument, NULL, 'a' },
>  		{ "capture-comment", required_argument, NULL, 0 },
> +		{ "file-prefix",     required_argument, NULL, 0 },
>  		{ "help",            no_argument,       NULL, 'h' },
>  		{ "interface",       required_argument, NULL, 'i' },
>  		{ "list-interfaces", no_argument,       NULL, 'D' },
> @@ -330,11 +333,13 @@ static void parse_opts(int argc, char **argv)
> 
>  		switch (c) {
>  		case 0:
> -			switch (option_index) {
> -			case 0:
> +			if (!strcmp(long_options[option_index].name,
> +				    "capture-comment")) {
>  				capture_comment = optarg;
> -				break;
> -			default:
> +			} else if (!strcmp(long_options[option_index].name,
> +					   "file-prefix")) {
> +				file_prefix = optarg;
> +			} else {
>  				usage();
>  				exit(1);
>  			}

parse_opts() is called after dpdk_init(). So whatever file-prefix we provide, for eal init, it remains NULL.
Please let me know your thoughts about it.

> @@ -512,12 +517,14 @@ static void dpdk_init(void)
>  	static const char * const args[] = {
>  		"dumpcap", "--proc-type", "secondary",
>  		"--log-level", "notice"
> -
>  	};
> -	const int eal_argc = RTE_DIM(args);
> +	int eal_argc = RTE_DIM(args);
>  	char **eal_argv;
>  	unsigned int i;
> 
> +	if (file_prefix != NULL)
> +		eal_argc += 2;
> +
>  	/* DPDK API requires mutable versions of command line arguments.
> */
>  	eal_argv = calloc(eal_argc + 1, sizeof(char *));
>  	if (eal_argv == NULL)
> @@ -527,6 +534,11 @@ static void dpdk_init(void)
>  	for (i = 1; i < RTE_DIM(args); i++)
>  		eal_argv[i] = strdup(args[i]);
> 
> +	if (file_prefix != NULL) {
> +		eal_argv[i++] = strdup("--file-prefix");
> +		eal_argv[i++] = strdup(file_prefix);
> +	}
> +
>  	if (rte_eal_init(eal_argc, eal_argv) < 0)
>  		rte_exit(EXIT_FAILURE, "EAL init failed: is primary process
> running?\n");
> 
> --
> 2.35.1

Re: [RFT] dumpcap: add file-prefix option

[Ben Magistro]

[-- Attachment #1: Type: text/plain, Size: 4814 bytes --]

Kaur,

I believe parse_opts() should be called before dpdk_init() now see
https://patches.dpdk.org/project/dpdk/patch/20220125032545.7704-1-koncept1@gmail.com/

On Fri, Sep 16, 2022 at 4:19 AM Kaur, Arshdeep <arshdeep.kaur@intel.com>
wrote:

>
>
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, September 13, 2022 12:34 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur, Arshdeep
> > <arshdeep.kaur@intel.com>
> > Subject: [RFT] dumpcap: add file-prefix option
> >
> > When using dumpcap in container environment or with multiple DPDK
> > processes, it is useful to be able to specify file prefix.
> >
> > This version only accepts the long format option used by other commands.
> > If no prefix is specified then the default is used.
> >
> > Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > Did basic command line test, but still needs testing with a prefix being
> used
> > (ie multiple apps).
> >
> >  app/dumpcap/main.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c index
> > a6041d4ff495..bdeef96d9c0b 100644
> > --- a/app/dumpcap/main.c
> > +++ b/app/dumpcap/main.c
> > @@ -61,6 +61,7 @@ static char *output_name;  static const char
> > *filter_str;  static unsigned int ring_size = 2048;  static const char
> > *capture_comment;
> > +static const char *file_prefix;
> >  static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;  static bool
> > dump_bpf;  static struct { @@ -122,6 +123,7 @@ static void usage(void)
> >              "                           add a capture comment to the
> output file\n"
> >              "\n"
> >              "Miscellaneous:\n"
> > +            "  --file-prefix=<prefix>   prefix to use for
> multi-process\n"
> >              "  -q                       don't report packet capture
> counts\n"
> >              "  -v, --version            print version information and
> exit\n"
> >              "  -h, --help               display this help and exit\n"
> > @@ -310,6 +312,7 @@ static void parse_opts(int argc, char **argv)
> >       static const struct option long_options[] = {
> >               { "autostop",        required_argument, NULL, 'a' },
> >               { "capture-comment", required_argument, NULL, 0 },
> > +             { "file-prefix",     required_argument, NULL, 0 },
> >               { "help",            no_argument,       NULL, 'h' },
> >               { "interface",       required_argument, NULL, 'i' },
> >               { "list-interfaces", no_argument,       NULL, 'D' },
> > @@ -330,11 +333,13 @@ static void parse_opts(int argc, char **argv)
> >
> >               switch (c) {
> >               case 0:
> > -                     switch (option_index) {
> > -                     case 0:
> > +                     if (!strcmp(long_options[option_index].name,
> > +                                 "capture-comment")) {
> >                               capture_comment = optarg;
> > -                             break;
> > -                     default:
> > +                     } else if (!strcmp(long_options[option_index].name,
> > +                                        "file-prefix")) {
> > +                             file_prefix = optarg;
> > +                     } else {
> >                               usage();
> >                               exit(1);
> >                       }
>
> parse_opts() is called after dpdk_init(). So whatever file-prefix we
> provide, for eal init, it remains NULL.
> Please let me know your thoughts about it.
>
> > @@ -512,12 +517,14 @@ static void dpdk_init(void)
> >       static const char * const args[] = {
> >               "dumpcap", "--proc-type", "secondary",
> >               "--log-level", "notice"
> > -
> >       };
> > -     const int eal_argc = RTE_DIM(args);
> > +     int eal_argc = RTE_DIM(args);
> >       char **eal_argv;
> >       unsigned int i;
> >
> > +     if (file_prefix != NULL)
> > +             eal_argc += 2;
> > +
> >       /* DPDK API requires mutable versions of command line arguments.
> > */
> >       eal_argv = calloc(eal_argc + 1, sizeof(char *));
> >       if (eal_argv == NULL)
> > @@ -527,6 +534,11 @@ static void dpdk_init(void)
> >       for (i = 1; i < RTE_DIM(args); i++)
> >               eal_argv[i] = strdup(args[i]);
> >
> > +     if (file_prefix != NULL) {
> > +             eal_argv[i++] = strdup("--file-prefix");
> > +             eal_argv[i++] = strdup(file_prefix);
> > +     }
> > +
> >       if (rte_eal_init(eal_argc, eal_argv) < 0)
> >               rte_exit(EXIT_FAILURE, "EAL init failed: is primary process
> > running?\n");
> >
> > --
> > 2.35.1
>
>

[-- Attachment #2: Type: text/html, Size: 6990 bytes --]

Re: [RFT] dumpcap: add file-prefix option

[Stephen Hemminger]

On Fri, 16 Sep 2022 08:51:59 -0400
Ben Magistro <koncept1@gmail.com> wrote:

> Kaur,
> 
> I believe parse_opts() should be called before dpdk_init() now see
> https://patches.dpdk.org/project/dpdk/patch/20220125032545.7704-1-koncept1@gmail.com/

Correct, in main branch parse_opts is before dpdk_init 

RE: [RFT] dumpcap: add file-prefix option

[Kaur, Arshdeep]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, September 16, 2022 9:05 PM
> To: Ben Magistro <koncept1@gmail.com>
> Cc: Kaur, Arshdeep <arshdeep.kaur@intel.com>; dev@dpdk.org
> Subject: Re: [RFT] dumpcap: add file-prefix option
> 
> On Fri, 16 Sep 2022 08:51:59 -0400
> Ben Magistro <koncept1@gmail.com> wrote:
> 
> > Kaur,
> >
> > I believe parse_opts() should be called before dpdk_init() now see
> > https://patches.dpdk.org/project/dpdk/patch/20220125032545.7704-1-
> konc
> > ept1@gmail.com/
> 
> Correct, in main branch parse_opts is before dpdk_init

Hi Stephen and Ben. I have a doubt regarding this. According to me if dpdk_init is called after parse_opts, then some caller functions (called from within parse_opts) are affected.
Eg.	1) Parameter 'D' : { "list-interfaces", no_argument,       NULL, 'D' }, does not give any output.
	2) Parameter 'i' : { "interface",       required_argument, NULL, 'i' }, does not behave properly. 
I think the reason is that port list is available after eal init. Dumpcap(secondary process) will inherit the ports used by primary process.
This implies that dpdk_init should be called before parse_opts.
But then for multiprocess support, primary process is to be picked up by providing a file-prefix (Added in parse_opts).
This implies that parse_opts should be called before dpdk_init.

So according to me there was a deadlock situation here. Which I handled by providing file-prefix input to the program before dpdk_init manually. And then after dpdk_init, parse_opts should be called.

Please let me know if I am going in the wrong direction. And how else can we solve this?

NOTE: There is a bug in 'if' condition of select_interface function. Control should enter the 'if' condition if we provide select interface as '*' but instead it enters when they are unequal. So I have already submitted a patch for that. And it is acknowledged. Change done is:
- if (strcmp(arg, "*"))
+ if (!strcmp(arg, "*"))

RE: [RFT] dumpcap: add file-prefix option

[Kaur, Arshdeep]

++ Balaji and Mike.

> -----Original Message-----
> From: Kaur, Arshdeep <arshdeep.kaur@intel.com>
> Sent: Friday, September 16, 2022 1:49 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; dev@dpdk.org
> Subject: RE: [RFT] dumpcap: add file-prefix option
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, September 13, 2022 12:34 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur,
> Arshdeep
> > <arshdeep.kaur@intel.com>
> > Subject: [RFT] dumpcap: add file-prefix option
> >
> > When using dumpcap in container environment or with multiple DPDK
> > processes, it is useful to be able to specify file prefix.
> >
> > This version only accepts the long format option used by other
> commands.
> > If no prefix is specified then the default is used.
> >
> > Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > Did basic command line test, but still needs testing with a prefix
> > being used (ie multiple apps).
> >
> >  app/dumpcap/main.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c index
> > a6041d4ff495..bdeef96d9c0b 100644
> > --- a/app/dumpcap/main.c
> > +++ b/app/dumpcap/main.c
> > @@ -61,6 +61,7 @@ static char *output_name;  static const char
> > *filter_str;  static unsigned int ring_size = 2048;  static const char
> > *capture_comment;
> > +static const char *file_prefix;
> >  static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;  static bool
> > dump_bpf;  static struct { @@ -122,6 +123,7 @@ static void usage(void)
> >  	       "                           add a capture comment to the output file\n"
> >  	       "\n"
> >  	       "Miscellaneous:\n"
> > +	       "  --file-prefix=<prefix>   prefix to use for multi-process\n"
> >  	       "  -q                       don't report packet capture counts\n"
> >  	       "  -v, --version            print version information and exit\n"
> >  	       "  -h, --help               display this help and exit\n"
> > @@ -310,6 +312,7 @@ static void parse_opts(int argc, char **argv)
> >  	static const struct option long_options[] = {
> >  		{ "autostop",        required_argument, NULL, 'a' },
> >  		{ "capture-comment", required_argument, NULL, 0 },
> > +		{ "file-prefix",     required_argument, NULL, 0 },
> >  		{ "help",            no_argument,       NULL, 'h' },
> >  		{ "interface",       required_argument, NULL, 'i' },
> >  		{ "list-interfaces", no_argument,       NULL, 'D' },
> > @@ -330,11 +333,13 @@ static void parse_opts(int argc, char **argv)
> >
> >  		switch (c) {
> >  		case 0:
> > -			switch (option_index) {
> > -			case 0:
> > +			if (!strcmp(long_options[option_index].name,
> > +				    "capture-comment")) {
> >  				capture_comment = optarg;
> > -				break;
> > -			default:
> > +			} else if (!strcmp(long_options[option_index].name,
> > +					   "file-prefix")) {
> > +				file_prefix = optarg;
> > +			} else {
> >  				usage();
> >  				exit(1);
> >  			}
> 
> parse_opts() is called after dpdk_init(). So whatever file-prefix we provide,
> for eal init, it remains NULL.
> Please let me know your thoughts about it.
> 
> > @@ -512,12 +517,14 @@ static void dpdk_init(void)
> >  	static const char * const args[] = {
> >  		"dumpcap", "--proc-type", "secondary",
> >  		"--log-level", "notice"
> > -
> >  	};
> > -	const int eal_argc = RTE_DIM(args);
> > +	int eal_argc = RTE_DIM(args);
> >  	char **eal_argv;
> >  	unsigned int i;
> >
> > +	if (file_prefix != NULL)
> > +		eal_argc += 2;
> > +
> >  	/* DPDK API requires mutable versions of command line arguments.
> > */
> >  	eal_argv = calloc(eal_argc + 1, sizeof(char *));
> >  	if (eal_argv == NULL)
> > @@ -527,6 +534,11 @@ static void dpdk_init(void)
> >  	for (i = 1; i < RTE_DIM(args); i++)
> >  		eal_argv[i] = strdup(args[i]);
> >
> > +	if (file_prefix != NULL) {
> > +		eal_argv[i++] = strdup("--file-prefix");
> > +		eal_argv[i++] = strdup(file_prefix);
> > +	}
> > +
> >  	if (rte_eal_init(eal_argc, eal_argv) < 0)
> >  		rte_exit(EXIT_FAILURE, "EAL init failed: is primary process
> > running?\n");
> >
> > --
> > 2.35.1

RE: [RFT] dumpcap: add file-prefix option

[Kaur, Arshdeep]

Hi Stephen,

These changes are looking good as compared to 
http://patches.dpdk.org/project/dpdk/patch/20220912190330.73159-1-stephen@networkplumber.org/.
I have tested the changes. Works for me.
I am looking for this change in this release. Can you send the v1?

Thanks and regards,
Arshdeep Kaur

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, September 13, 2022 12:34 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur, Arshdeep
> <arshdeep.kaur@intel.com>
> Subject: [RFT] dumpcap: add file-prefix option
> 
> When using dumpcap in container environment or with multiple DPDK
> processes, it is useful to be able to specify file prefix.
> 
> This version only accepts the long format option used by other commands.
> If no prefix is specified then the default is used.
> 
> Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> Did basic command line test, but still needs testing with a prefix being used
> (ie multiple apps).
> 
>  app/dumpcap/main.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c index
> a6041d4ff495..bdeef96d9c0b 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -61,6 +61,7 @@ static char *output_name;  static const char
> *filter_str;  static unsigned int ring_size = 2048;  static const char
> *capture_comment;
> +static const char *file_prefix;
>  static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;  static bool
> dump_bpf;  static struct { @@ -122,6 +123,7 @@ static void usage(void)
>  	       "                           add a capture comment to the output file\n"
>  	       "\n"
>  	       "Miscellaneous:\n"
> +	       "  --file-prefix=<prefix>   prefix to use for multi-process\n"
>  	       "  -q                       don't report packet capture counts\n"
>  	       "  -v, --version            print version information and exit\n"
>  	       "  -h, --help               display this help and exit\n"
> @@ -310,6 +312,7 @@ static void parse_opts(int argc, char **argv)
>  	static const struct option long_options[] = {
>  		{ "autostop",        required_argument, NULL, 'a' },
>  		{ "capture-comment", required_argument, NULL, 0 },
> +		{ "file-prefix",     required_argument, NULL, 0 },
>  		{ "help",            no_argument,       NULL, 'h' },
>  		{ "interface",       required_argument, NULL, 'i' },
>  		{ "list-interfaces", no_argument,       NULL, 'D' },
> @@ -330,11 +333,13 @@ static void parse_opts(int argc, char **argv)
> 
>  		switch (c) {
>  		case 0:
> -			switch (option_index) {
> -			case 0:
> +			if (!strcmp(long_options[option_index].name,
> +				    "capture-comment")) {
>  				capture_comment = optarg;
> -				break;
> -			default:
> +			} else if (!strcmp(long_options[option_index].name,
> +					   "file-prefix")) {
> +				file_prefix = optarg;
> +			} else {
>  				usage();
>  				exit(1);
>  			}
> @@ -512,12 +517,14 @@ static void dpdk_init(void)
>  	static const char * const args[] = {
>  		"dumpcap", "--proc-type", "secondary",
>  		"--log-level", "notice"
> -
>  	};
> -	const int eal_argc = RTE_DIM(args);
> +	int eal_argc = RTE_DIM(args);
>  	char **eal_argv;
>  	unsigned int i;
> 
> +	if (file_prefix != NULL)
> +		eal_argc += 2;
> +
>  	/* DPDK API requires mutable versions of command line arguments.
> */
>  	eal_argv = calloc(eal_argc + 1, sizeof(char *));
>  	if (eal_argv == NULL)
> @@ -527,6 +534,11 @@ static void dpdk_init(void)
>  	for (i = 1; i < RTE_DIM(args); i++)
>  		eal_argv[i] = strdup(args[i]);
> 
> +	if (file_prefix != NULL) {
> +		eal_argv[i++] = strdup("--file-prefix");
> +		eal_argv[i++] = strdup(file_prefix);
> +	}
> +
>  	if (rte_eal_init(eal_argc, eal_argv) < 0)
>  		rte_exit(EXIT_FAILURE, "EAL init failed: is primary process
> running?\n");
> 
> --
> 2.35.1

RE: [RFT] dumpcap: add file-prefix option

[Kaur, Arshdeep]

Hi Stephen, a gentle reminder.

Thanks and regards,
Arshdeep Kaur

> -----Original Message-----
> From: Kaur, Arshdeep
> Sent: Monday, October 17, 2022 10:38 AM
> To: Stephen Hemminger <stephen@networkplumber.org>; dev@dpdk.org
> Cc: Chintalapalle, Balaji <balaji.chintalapalle@intel.com>; Beadle, Michael
> <michael.beadle@intel.com>
> Subject: RE: [RFT] dumpcap: add file-prefix option
> 
> Hi Stephen,
> 
> These changes are looking good as compared to
> http://patches.dpdk.org/project/dpdk/patch/20220912190330.73159-1-
> stephen@networkplumber.org/.
> I have tested the changes. Works for me.
> I am looking for this change in this release. Can you send the v1?
> 
> Thanks and regards,
> Arshdeep Kaur
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, September 13, 2022 12:34 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur,
> Arshdeep
> > <arshdeep.kaur@intel.com>
> > Subject: [RFT] dumpcap: add file-prefix option
> >
> > When using dumpcap in container environment or with multiple DPDK
> > processes, it is useful to be able to specify file prefix.
> >
> > This version only accepts the long format option used by other
> commands.
> > If no prefix is specified then the default is used.
> >
> > Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > Did basic command line test, but still needs testing with a prefix
> > being used (ie multiple apps).
> >
> >  app/dumpcap/main.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c index
> > a6041d4ff495..bdeef96d9c0b 100644
> > --- a/app/dumpcap/main.c
> > +++ b/app/dumpcap/main.c
> > @@ -61,6 +61,7 @@ static char *output_name;  static const char
> > *filter_str;  static unsigned int ring_size = 2048;  static const char
> > *capture_comment;
> > +static const char *file_prefix;
> >  static uint32_t snaplen = RTE_MBUF_DEFAULT_BUF_SIZE;  static bool
> > dump_bpf;  static struct { @@ -122,6 +123,7 @@ static void usage(void)
> >  	       "                           add a capture comment to the output file\n"
> >  	       "\n"
> >  	       "Miscellaneous:\n"
> > +	       "  --file-prefix=<prefix>   prefix to use for multi-process\n"
> >  	       "  -q                       don't report packet capture counts\n"
> >  	       "  -v, --version            print version information and exit\n"
> >  	       "  -h, --help               display this help and exit\n"
> > @@ -310,6 +312,7 @@ static void parse_opts(int argc, char **argv)
> >  	static const struct option long_options[] = {
> >  		{ "autostop",        required_argument, NULL, 'a' },
> >  		{ "capture-comment", required_argument, NULL, 0 },
> > +		{ "file-prefix",     required_argument, NULL, 0 },
> >  		{ "help",            no_argument,       NULL, 'h' },
> >  		{ "interface",       required_argument, NULL, 'i' },
> >  		{ "list-interfaces", no_argument,       NULL, 'D' },
> > @@ -330,11 +333,13 @@ static void parse_opts(int argc, char **argv)
> >
> >  		switch (c) {
> >  		case 0:
> > -			switch (option_index) {
> > -			case 0:
> > +			if (!strcmp(long_options[option_index].name,
> > +				    "capture-comment")) {
> >  				capture_comment = optarg;
> > -				break;
> > -			default:
> > +			} else if (!strcmp(long_options[option_index].name,
> > +					   "file-prefix")) {
> > +				file_prefix = optarg;
> > +			} else {
> >  				usage();
> >  				exit(1);
> >  			}
> > @@ -512,12 +517,14 @@ static void dpdk_init(void)
> >  	static const char * const args[] = {
> >  		"dumpcap", "--proc-type", "secondary",
> >  		"--log-level", "notice"
> > -
> >  	};
> > -	const int eal_argc = RTE_DIM(args);
> > +	int eal_argc = RTE_DIM(args);
> >  	char **eal_argv;
> >  	unsigned int i;
> >
> > +	if (file_prefix != NULL)
> > +		eal_argc += 2;
> > +
> >  	/* DPDK API requires mutable versions of command line arguments.
> > */
> >  	eal_argv = calloc(eal_argc + 1, sizeof(char *));
> >  	if (eal_argv == NULL)
> > @@ -527,6 +534,11 @@ static void dpdk_init(void)
> >  	for (i = 1; i < RTE_DIM(args); i++)
> >  		eal_argv[i] = strdup(args[i]);
> >
> > +	if (file_prefix != NULL) {
> > +		eal_argv[i++] = strdup("--file-prefix");
> > +		eal_argv[i++] = strdup(file_prefix);
> > +	}
> > +
> >  	if (rte_eal_init(eal_argc, eal_argv) < 0)
> >  		rte_exit(EXIT_FAILURE, "EAL init failed: is primary process
> > running?\n");
> >
> > --
> > 2.35.1

Re: [RFT] dumpcap: add file-prefix option

[Stephen Hemminger]

On Thu, 20 Oct 2022 10:43:28 +0000
"Kaur, Arshdeep" <arshdeep.kaur@intel.com> wrote:

> Hi Stephen, a gentle reminder.
> 
> Thanks and regards,
> Arshdeep Kaur
> 
> > -----Original Message-----
> > From: Kaur, Arshdeep
> > Sent: Monday, October 17, 2022 10:38 AM
> > To: Stephen Hemminger <stephen@networkplumber.org>; dev@dpdk.org
> > Cc: Chintalapalle, Balaji <balaji.chintalapalle@intel.com>; Beadle, Michael
> > <michael.beadle@intel.com>
> > Subject: RE: [RFT] dumpcap: add file-prefix option
> > 
> > Hi Stephen,
> > 
> > These changes are looking good as compared to
> > http://patches.dpdk.org/project/dpdk/patch/20220912190330.73159-1-
> > stephen@networkplumber.org/.
> > I have tested the changes. Works for me.
> > I am looking for this change in this release. Can you send the v1?
> > 
> > Thanks and regards,
> > Arshdeep Kaur
> >   
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Tuesday, September 13, 2022 12:34 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur,  
> > Arshdeep  
> > > <arshdeep.kaur@intel.com>
> > > Subject: [RFT] dumpcap: add file-prefix option
> > >
> > > When using dumpcap in container environment or with multiple DPDK
> > > processes, it is useful to be able to specify file prefix.
> > >
> > > This version only accepts the long format option used by other  
> > commands.  
> > > If no prefix is specified then the default is used.
> > >
> > > Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---

Yes, these should be ready to merge.
This is busy time in release cycle for David and Thomas.

RE: [RFT] dumpcap: add file-prefix option

[Kaur, Arshdeep]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, September 13, 2022 12:34 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Kaur, Arshdeep
> <arshdeep.kaur@intel.com>
> Subject: [RFT] dumpcap: add file-prefix option
> 
> When using dumpcap in container environment or with multiple DPDK
> processes, it is useful to be able to specify file prefix.
> 
> This version only accepts the long format option used by other commands.
> If no prefix is specified then the default is used.
> 
> Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Looks good ready to merge.

Acked-by:  Arshdeep Kaur <arshdeep.kaur@intel.com>

Tested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>

Re: [RFT] dumpcap: add file-prefix option

[David Marchand]

On Thu, Oct 20, 2022 at 1:55 PM Kaur, Arshdeep <arshdeep.kaur@intel.com> wrote:
> >
> > When using dumpcap in container environment or with multiple DPDK
> > processes, it is useful to be able to specify file prefix.
> >
> > This version only accepts the long format option used by other commands.
> > If no prefix is specified then the default is used.
> >
> > Suggested-by: Arshdeep Kaur <arshdeep.kaur@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by:  Arshdeep Kaur <arshdeep.kaur@intel.com>

Applied, thanks.


-- 
David Marchand