[dpdk-dev] [PATCH 0/2] app/testpmd: add portlist option to the testpmd

[dpdk-dev] [PATCH 0/2] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

This patchset contains 2 patches.
The first one will be changes to testpmd where a new option
portlist is added. 
The second one adds a function eal_parse_optionlist which will
parse user input

The following new command line parameter is added:
--portlist: user can specify the list of forwarding ports
In current version, we are setting the ports using portmask.
With portmask, we can use only upto 64 ports. 
This portlist option enables the user to use more than 64 ports.
        --portlist <p1>[-p2][,p3[-p4],...]

The following new API is added:
eal_parse_optionlist()
In current version, there is a function which parses
the corelist based on user value. A new generic
function eal_parse_optionlist is added which will
parse corelist as well as similar user input from
the command line

Hariprasad Govindharajan (2):
  app/testpmd: add portlist option to the testpmd
  eal: add eal_parse_optionlist to parse user input

 app/test-pmd/config.c                      | 25 +++++++++++++++++
 app/test-pmd/parameters.c                  |  5 ++++
 app/test-pmd/testpmd.h                     |  3 ++
 lib/librte_eal/common/eal_common_options.c | 45 ++++++++++++++++++------------
 lib/librte_eal/common/include/rte_eal.h    | 34 ++++++++++++++++++++++
 lib/librte_eal/rte_eal_version.map         |  1 +
 6 files changed, 95 insertions(+), 18 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/2] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

    In current version, we are setting the ports
    using portmask. With portmask, we can use only
    upto 64 ports. This portlist option enables the user
    to use more than 64 ports.
    Now we can specify the ports in 2 different ways
    - Using portmask (-p [0x]nnn): mask must be in hex format
    - Using portlist in the following format
      --portlist <p1>[-p2][,p3[-p4],...]

    --portmask 0x2 is same as --portlist 1
    --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
 app/test-pmd/config.c     | 25 +++++++++++++++++++++++++
 app/test-pmd/parameters.c |  5 +++++
 app/test-pmd/testpmd.h    |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..49662cf 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2588,6 +2588,31 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 }
 
 void
+parse_fwd_portlist(const char *portlist)
+{
+	unsigned int portcount = 0;
+	int portindex[RTE_MAX_ETHPORTS];
+	unsigned int ports[RTE_MAX_ETHPORTS];
+	unsigned int idx;
+	/*
+	 * eal_parse_portlist() will mark the portindex array
+	 * with -1 if the port is not listed and with a positive value
+	 * for the listed ports. however, the function set_fwd_ports_list()
+	 * requires a list of portids' so we use 2 arrays to do the
+	 * conversion between 2 formats.
+	 */
+	if (eal_parse_optionlist(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	RTE_ETH_FOREACH_DEV(idx) {
+		if (portindex[idx] != -1)
+			ports[portcount++] = idx;
+	}
+
+	set_fwd_ports_list(ports, portcount);
+}
+
+void
 set_fwd_ports_mask(uint64_t portmask)
 {
 	unsigned int portlist[64];
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
-- 
2.7.4

[dpdk-dev] [PATCH v2] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

        In current version, we are setting the ports
        using portmask. With portmask, we can use only
        upto 64 ports. This portlist option enables the user
        to use more than 64 ports.
        Now we can specify the ports in 2 different ways
        - Using portmask (-p [0x]nnn): mask must be in hex format
        - Using portlist in the following format
          --portlist <p1>[-p2][,p3[-p4],...]

        --portmask 0x2 is same as --portlist 1
        --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
 app/test-pmd/config.c                      | 57 +++++++++++++++++++++++++++++-
 lib/librte_eal/common/eal_common_options.c | 45 ++++++++++-------------
 lib/librte_eal/common/include/rte_eal.h    | 34 ------------------
 lib/librte_eal/rte_eal_version.map         |  1 -
 4 files changed, 74 insertions(+), 63 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 49662cf..c10aa78 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2587,6 +2587,61 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+static int parse_port_list(const char *list, int *values, int maxsize)
+{
+	unsigned int count = 0;
+	char *end = NULL;
+	int min, max;
+	int idx;
+
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	for (idx = 0; idx < maxsize; idx++)
+		values[idx] = -1;
+
+	/* Remove all blank characters ahead */
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
+
+	do {
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			return -1;
+		errno = 0;
+		idx = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (idx < 0 || idx >= maxsize)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = idx;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = idx;
+			if (min == maxsize)
+				min = idx;
+			for (idx = min; idx <= max; idx++) {
+				if (values[idx] == -1) {
+					values[idx] = count;
+					count++;
+				}
+			}
+			min = maxsize;
+		} else
+			return -1;
+		list = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+	return 0;
+}
+
 void
 parse_fwd_portlist(const char *portlist)
 {
@@ -2601,7 +2656,7 @@ parse_fwd_portlist(const char *portlist)
 	 * requires a list of portids' so we use 2 arrays to do the
 	 * conversion between 2 formats.
 	 */
-	if (eal_parse_optionlist(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
+	if (parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
 		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
 
 	RTE_ETH_FOREACH_DEV(idx) {
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index aa59f8b..5920233 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -571,36 +571,33 @@ eal_parse_service_corelist(const char *corelist)
 	return 0;
 }
 
-int
-eal_parse_optionlist(const char *list, int *values, int maxsize)
+static int
+eal_parse_corelist(const char *corelist, int *cores)
 {
 	unsigned count = 0;
 	char *end = NULL;
 	int min, max;
 	int idx;
 
-	if (list == NULL || values == NULL || maxsize < 0)
-		return -1;
-
-	for (idx = 0; idx < maxsize; idx++)
-		values[idx] = -1;
+	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
+		cores[idx] = -1;
 
 	/* Remove all blank characters ahead */
-	while (isblank(*list))
-		list++;
-
-	min = maxsize;
+	while (isblank(*corelist))
+		corelist++;
 
+	/* Get list of cores */
+	min = RTE_MAX_LCORE;
 	do {
-		while (isblank(*list))
-			list++;
-		if (*list == '\0')
+		while (isblank(*corelist))
+			corelist++;
+		if (*corelist == '\0')
 			return -1;
 		errno = 0;
-		idx = strtol(list, &end, 10);
+		idx = strtol(corelist, &end, 10);
 		if (errno || end == NULL)
 			return -1;
-		if (idx < 0 || idx >= maxsize)
+		if (idx < 0 || idx >= RTE_MAX_LCORE)
 			return -1;
 		while (isblank(*end))
 			end++;
@@ -608,18 +605,18 @@ eal_parse_optionlist(const char *list, int *values, int maxsize)
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == maxsize)
+			if (min == RTE_MAX_LCORE)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (values[idx] == -1) {
-					values[idx] = count;
+				if (cores[idx] == -1) {
+					cores[idx] = count;
 					count++;
 				}
 			}
-			min = maxsize;
+			min = RTE_MAX_LCORE;
 		} else
 			return -1;
-		list = end + 1;
+		corelist = end + 1;
 	} while (*end != '\0');
 
 	if (count == 0)
@@ -627,12 +624,6 @@ eal_parse_optionlist(const char *list, int *values, int maxsize)
 	return 0;
 }
 
-static int
-eal_parse_corelist(const char *corelist, int *cores)
-{
-	return eal_parse_optionlist(corelist, cores, RTE_MAX_LCORE);
-}
-
 /* Changes the lcore id of the master thread */
 static int
 eal_parse_master_lcore(const char *arg)
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 567b754..2f9ed29 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -71,40 +71,6 @@ enum rte_proc_type_t rte_eal_process_type(void);
 int rte_eal_iopl_init(void);
 
 /**
- * Parse the user input
- *
- * This function can be used to read and parse the user input
- * from the command line. For example, when the user specifies
- * corelist or port list this function will read the input
- * and set the forwarding cores or ports
- *
- * @param[in] list
- *   String containing the user input. User can specify
- *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
- *   For example, if the user wants to use all the available
- *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
- *   If the user wants to use only the ports 1,2 then the input
- *   is 1,2.
- *   valid characters are '-' and ','
- *   invalid chars like '.' or '#' will result in
- *   EAL: Error - exiting with code: 1
- *     Cause: Invalid fwd port list
- * @param[in] values
- *   An array pointer, used by this function to set the
- *   array contents to a positive value if they are listed
- *   in the input
- *   else sets it to -1
- * @param[in] maxsize
- *   This is the maximum value the list string can contain
- * @return
- *   -On success, returns 0.
- *   -On failure, returns -1.
- */
-__rte_experimental
-int
-eal_parse_optionlist(const char *list, int *values, int maxsize);
-
-/**
  * Initialize the Environment Abstraction Layer (EAL).
  *
  * This function is to be executed on the MASTER lcore only, as soon
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 3d72df8..e38d025 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -332,5 +332,4 @@ EXPERIMENTAL {
 	# added in 19.11
 	rte_log_get_stream;
 	rte_mcfg_get_single_file_segments;
-	eal_parse_optionlist;
 };
-- 
2.7.4

[dpdk-dev] [PATCH v3] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

    In current version, we are setting the ports
    using portmask. With portmask, we can use only
    upto 64 ports. This portlist option enables the user
    to use more than 64 ports.
    Now we can specify the ports in 2 different ways
    - Using portmask (-p [0x]nnn): mask must be in hex format
    - Using portlist in the following format
      --portlist <p1>[-p2][,p3[-p4],...]

    --portmask 0x2 is same as --portlist 1
    --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser
v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c     | 80 +++++++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/parameters.c |  5 +++
 app/test-pmd/testpmd.h    |  3 ++
 3 files changed, 88 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..c10aa78 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2587,6 +2587,86 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+static int parse_port_list(const char *list, int *values, int maxsize)
+{
+	unsigned int count = 0;
+	char *end = NULL;
+	int min, max;
+	int idx;
+
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	for (idx = 0; idx < maxsize; idx++)
+		values[idx] = -1;
+
+	/* Remove all blank characters ahead */
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
+
+	do {
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			return -1;
+		errno = 0;
+		idx = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (idx < 0 || idx >= maxsize)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = idx;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = idx;
+			if (min == maxsize)
+				min = idx;
+			for (idx = min; idx <= max; idx++) {
+				if (values[idx] == -1) {
+					values[idx] = count;
+					count++;
+				}
+			}
+			min = maxsize;
+		} else
+			return -1;
+		list = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+	return 0;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	unsigned int portcount = 0;
+	int portindex[RTE_MAX_ETHPORTS];
+	unsigned int ports[RTE_MAX_ETHPORTS];
+	unsigned int idx;
+	/*
+	 * eal_parse_portlist() will mark the portindex array
+	 * with -1 if the port is not listed and with a positive value
+	 * for the listed ports. however, the function set_fwd_ports_list()
+	 * requires a list of portids' so we use 2 arrays to do the
+	 * conversion between 2 formats.
+	 */
+	if (parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	RTE_ETH_FOREACH_DEV(idx) {
+		if (portindex[idx] != -1)
+			ports[portcount++] = idx;
+	}
+
+	set_fwd_ports_list(ports, portcount);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
-- 
2.7.4

[dpdk-dev] [PATCH v4] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

    In current version, we are setting the ports
    using portmask. With portmask, we can use only
    upto 64 ports. This portlist option enables the user
    to use more than 64 ports.
    Now we can specify the ports in 2 different ways
    - Using portmask (-p [0x]nnn): mask must be in hex format
    - Using portlist in the following format
      --portlist <p1>[-p2][,p3[-p4],...]

    --portmask 0x2 is same as --portlist 1
    --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values
v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser
v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c                 | 83 ++++++++++++++++++++++++++++++++++-
 app/test-pmd/parameters.c             |  5 +++
 app/test-pmd/testpmd.h                |  3 ++
 doc/guides/testpmd_app_ug/run_app.rst |  4 ++
 4 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..e55964e 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2564,7 +2564,6 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	unsigned int i;
 	portid_t port_id;
 	int record_now;
-
 	record_now = 0;
  again:
 	for (i = 0; i < nb_pt; i++) {
@@ -2587,6 +2586,88 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+static int
+parse_port_list(const char *list, int *values, int maxsize)
+{
+	unsigned int count = 0;
+	char *end = NULL;
+	int min, max;
+	int idx;
+
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	for (idx = 0; idx < maxsize; idx++)
+		values[idx] = -1;
+	/* Remove all blank characters ahead */
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
+
+	do {
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			return -1;
+		errno = 0;
+		idx = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (idx < 0 || idx >= maxsize)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = idx;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = idx;
+			if (min == maxsize)
+				min = idx;
+			for (idx = min; idx <= max; idx++) {
+				if (values[count] == -1) {
+					values[count] = idx;
+					count++;
+				}
+			}
+			min = maxsize;
+		} else
+			return -1;
+		list = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+	return count;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	int portcount = 0;
+	int portindex[RTE_MAX_ETHPORTS];
+	unsigned int idx;
+
+	/*
+	 * parse_port_list() will mark the portindex array
+	 * with -1 if the port is not listed and with a positive value
+	 * for the listed ports. So, the parser is designed in
+	 * such a way that it will fill the portindex array with the
+	 * valid ports from the user,and the function set_fwd_ports_list()
+	 * will set those ports in the forwarding mode
+	 */
+
+	if (parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	RTE_ETH_FOREACH_DEV(idx) {
+		if (portindex[idx] != -1)
+			portcount++;
+	}
+	printf("portcount = %d\n", portcount);
+	set_fwd_ports_list((unsigned int *)portindex, portcount);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 9ab4d70..655d194 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -77,6 +77,10 @@ The command line options are:
 
     Set the hexadecimal bitmask of the ports used by the packet forwarding test.
 
+*   ``--portlist=0-X`` or ``--portlist=X,X`` or ``--portlist=0-X,X``
+
+      Set the forwarding ports based on the user input used by the packet forwarding test.
+
 *   ``--numa``
 
     Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
-- 
2.7.4

[dpdk-dev] [PATCH v5] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

    In current version, we are setting the ports
    using portmask. With portmask, we can use only
    upto 64 ports. This portlist option enables the user
    to use more than 64 ports.
    Now we can specify the ports in 2 different ways
    - Using portmask (-p [0x]nnn): mask must be in hex format
    - Using portlist in the following format
      --portlist <p1>[-p2][,p3[-p4],...]

    --portmask 0x2 is same as --portlist 1
    --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v5:
added a check to validate the ports available before
setting them. also added comments in the testpmd file
for the new function

v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values

v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser

v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c                 | 107 +++++++++++++++++++++++++++++++++-
 app/test-pmd/parameters.c             |   5 ++
 app/test-pmd/testpmd.h                |   3 +
 doc/guides/testpmd_app_ug/run_app.rst |   4 ++
 4 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..6415022 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2564,7 +2564,6 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	unsigned int i;
 	portid_t port_id;
 	int record_now;
-
 	record_now = 0;
  again:
 	for (i = 0; i < nb_pt; i++) {
@@ -2587,6 +2586,112 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+/**
+ * Parse and obtain the list of forwarding ports
+ * from the user input
+ *
+ * @param[in] list
+ *   String containing the user input. User can specify
+ *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ *   For example, if the user wants to use all the available
+ *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ *   If the user wants to use only the ports 1,2 then the input
+ *   is 1,2.
+ *   valid characters are '-' and ','
+ *   invalid chars like '.' or '#' will result in
+ *   EAL: Error - exiting with code: 1
+ *     Cause: Invalid fwd port list
+ * @param[in] values
+ *   An array pointer, used by this function to set the
+ *   array contents to a positive value if they are listed
+ *   in the input else sets it to -1
+ * @param[in] maxsize
+ *   This is the maximum value the list string can contain
+ * @return
+ *   -On success, returns 0.
+ *   -On failure, returns -1.
+ */
+static int
+parse_port_list(const char *list, int *values, int maxsize)
+{
+	unsigned int count = 0;
+	char *end = NULL;
+	int min, max;
+	int idx;
+
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	for (idx = 0; idx < maxsize; idx++)
+		values[idx] = -1;
+	/* Remove all blank characters ahead */
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
+
+	do {
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			return -1;
+		errno = 0;
+		idx = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (idx < 0 || idx >= maxsize)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = idx;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = idx;
+			if (min == maxsize)
+				min = idx;
+			for (idx = min; idx <= max; idx++) {
+				if (values[count] == -1) {
+					values[count] = idx;
+					count++;
+				}
+			}
+			min = maxsize;
+		} else
+			return -1;
+		list = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+	return 0;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	int portcount = 0;
+	int portindex[RTE_MAX_ETHPORTS];
+	unsigned int idx;
+
+	/*
+	 * parse_port_list() will mark the portindex array
+	 * with -1 if the port is not listed and with a positive value
+	 * for the listed ports. So, the parser is designed in
+	 * such a way that it will fill the portindex array with the
+	 * valid ports from the user,and the function set_fwd_ports_list()
+	 * will set those ports in the forwarding mode
+	 */
+
+	if (parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	RTE_ETH_FOREACH_DEV(idx) {
+		if (rte_eth_dev_is_valid_port(portindex[idx]))
+			portcount++;
+	}
+	set_fwd_ports_list((unsigned int *)portindex, portcount);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 9ab4d70..5e8d379 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -77,6 +77,10 @@ The command line options are:
 
     Set the hexadecimal bitmask of the ports used by the packet forwarding test.
 
+*   ``--portlist=X``
+
+      Set the forwarding ports based on the user input used by the packet forwarding test.
+
 *   ``--numa``
 
     Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
-- 
2.7.4

Re: [dpdk-dev] [PATCH v5] app/testpmd: add portlist option to the testpmd

[Ferruh Yigit]

On 2/4/2020 4:48 PM, Hariprasad Govindharajan wrote:
>     In current version, we are setting the ports
>     using portmask. With portmask, we can use only
>     upto 64 ports. This portlist option enables the user
>     to use more than 64 ports.
>     Now we can specify the ports in 2 different ways
>     - Using portmask (-p [0x]nnn): mask must be in hex format
>     - Using portlist in the following format
>       --portlist <p1>[-p2][,p3[-p4],...]
> 
>     --portmask 0x2 is same as --portlist 1
>     --portmask 0x3 is same as --portlist 0-1
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
> ---
> v5:
> added a check to validate the ports available before
> setting them. also added comments in the testpmd file
> for the new function
> 
> v4:
> the parser is modified so that we don't ues 2 arrays
> to convert the listed port values
> 
> v3:
> squashed the 2 patches and made it 1 patch with
> changes only in testpmd. Also working on optmizing
> the parser
> 
> v2:
> moved the parser function to testpmd
> ---
>  app/test-pmd/config.c                 | 107 +++++++++++++++++++++++++++++++++-
>  app/test-pmd/parameters.c             |   5 ++
>  app/test-pmd/testpmd.h                |   3 +
>  doc/guides/testpmd_app_ug/run_app.rst |   4 ++
>  4 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 9669cbd..6415022 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2564,7 +2564,6 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
>  	unsigned int i;
>  	portid_t port_id;
>  	int record_now;
> -
>  	record_now = 0;
>   again:
>  	for (i = 0; i < nb_pt; i++) {
> @@ -2587,6 +2586,112 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
>  	}
>  }
>  
> +/**
> + * Parse and obtain the list of forwarding ports
> + * from the user input
> + *
> + * @param[in] list
> + *   String containing the user input. User can specify
> + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + *   For example, if the user wants to use all the available
> + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + *   If the user wants to use only the ports 1,2 then the input
> + *   is 1,2.
> + *   valid characters are '-' and ','
> + *   invalid chars like '.' or '#' will result in
> + *   EAL: Error - exiting with code: 1
> + *     Cause: Invalid fwd port list
> + * @param[in] values

Isn't values and output parameter, filled by funciton.

> + *   An array pointer, used by this function to set the

Above line is redundant information

> + *   array contents to a positive value if they are listed
> + *   in the input else sets it to -1

Can you please give more detail how the content of this array will be after this
function. Like is the parsed port valued used as index in the array, is the '-1'
list terminator or can be anywhere. Is the "positive value" used that index has
a valid port or that value is the port number etc...


> + * @param[in] maxsize
> + *   This is the maximum value the list string can contain

This is also the size of the 'values' array right? At least code uses it that
way, can you document this expectation?

> + * @return
> + *   -On success, returns 0.
> + *   -On failure, returns -1.
> + */
> +static int
> +parse_port_list(const char *list, int *values, int maxsize)
> +{
> +	unsigned int count = 0;
> +	char *end = NULL;
> +	int min, max;
> +	int idx;
> +
> +	if (list == NULL || values == NULL || maxsize < 0)
> +		return -1;
> +
> +	for (idx = 0; idx < maxsize; idx++)
> +		values[idx] = -1;
> +	/* Remove all blank characters ahead */
> +	while (isblank(*list))
> +		list++;
> +
> +	min = maxsize;
> +
> +	do {
> +		while (isblank(*list))
> +			list++;
> +		if (*list == '\0')
> +			return -1;
> +		errno = 0;
> +		idx = strtol(list, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		if (idx < 0 || idx >= maxsize)
> +			return -1;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = idx;
> +		} else if ((*end == ',') || (*end == '\0')) {
> +			max = idx;
> +			if (min == maxsize)
> +				min = idx;
> +			for (idx = min; idx <= max; idx++) {
> +				if (values[count] == -1) {
> +					values[count] = idx;
> +					count++;

Isn't there any check for the 'count', if the user provides something like
"1-30,2-29" won't this overflow and trash the stack?

Also there is no check to prevent the duplication, is it allowed, like
"1,1,1,1,1,1,1,1,1,1" ?

> +				}
> +			}
> +			min = maxsize;
> +		} else
> +			return -1;
> +		list = end + 1;
> +	} while (*end != '\0');
> +
> +	if (count == 0)
> +		return -1;
> +	return 0;
> +}
> +
> +void
> +parse_fwd_portlist(const char *portlist)
> +{
> +	int portcount = 0;
> +	int portindex[RTE_MAX_ETHPORTS];
> +	unsigned int idx;
> +
> +	/*
> +	 * parse_port_list() will mark the portindex array
> +	 * with -1 if the port is not listed and with a positive value
> +	 * for the listed ports. So, the parser is designed in
> +	 * such a way that it will fill the portindex array with the
> +	 * valid ports from the user,and the function set_fwd_ports_list()
> +	 * will set those ports in the forwarding mode
> +	 */
> +
> +	if (parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS) < 0)
> +		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
> +
> +	RTE_ETH_FOREACH_DEV(idx) {
> +		if (rte_eth_dev_is_valid_port(portindex[idx]))
> +			portcount++;

This looks long, 'portcount' takes the valid ports in all list, but
'set_fwd_ports_list()' processes only first 'portcount' ports, so a value like
following will fail if there are only two valid ports (0,1):
"5-10,0,1"

> +	}
> +	set_fwd_ports_list((unsigned int *)portindex, portcount);
> +}
> +
>  void
>  set_fwd_ports_mask(uint64_t portmask)
>  {
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 6340104..404dba2 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -57,6 +57,7 @@ usage(char* progname)
>  	       "[--help|-h] | [--auto-start|-a] | ["
>  	       "--tx-first | --stats-period=PERIOD | "
>  	       "--coremask=COREMASK --portmask=PORTMASK --numa "
> +	       "--portlist=PORTLIST "
>  	       "--mbuf-size= | --total-num-mbufs= | "
>  	       "--nb-cores= | --nb-ports= | "
>  #ifdef RTE_LIBRTE_CMDLINE
> @@ -92,6 +93,7 @@ usage(char* progname)
>  	       "packet forwarding.\n");
>  	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
>  	       "by the packet forwarding test.\n");
> +	printf("  --portlist=PORTLIST: list of forwarding ports\n");
>  	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
>  	       "RX memory buffers (mbufs).\n");
>  	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
> @@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
>  		{ "nb-ports",			1, 0, 0 },
>  		{ "coremask",			1, 0, 0 },
>  		{ "portmask",			1, 0, 0 },
> +		{ "portlist",			1, 0, 0 },
>  		{ "numa",			0, 0, 0 },
>  		{ "no-numa",			0, 0, 0 },
>  		{ "mp-anon",			0, 0, 0 },
> @@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
>  				parse_fwd_coremask(optarg);
>  			if (!strcmp(lgopts[opt_idx].name, "portmask"))
>  				parse_fwd_portmask(optarg);
> +			if (!strcmp(lgopts[opt_idx].name, "portlist"))
> +				parse_fwd_portlist(optarg);
>  			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
>  				numa_support = 0;
>  			if (!strcmp(lgopts[opt_idx].name, "numa"))
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 3dd5fc7..33ef3e2 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -614,6 +614,9 @@ lcore_num(void)
>  	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
>  }
>  
> +void
> +parse_fwd_portlist(const char *port);
> +
>  static inline struct fwd_lcore *
>  current_fwd_lcore(void)
>  {
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index 9ab4d70..5e8d379 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -77,6 +77,10 @@ The command line options are:
>  
>      Set the hexadecimal bitmask of the ports used by the packet forwarding test.
>  
> +*   ``--portlist=X``
> +
> +      Set the forwarding ports based on the user input used by the packet forwarding test.

Can you please document the expected syntax of the "X" briefly, like in a
sentences, here?

> +
>  *   ``--numa``
>  
>      Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
> 

[dpdk-dev] [PATCH v6] app/testpmd: add portlist option to the testpmd

[Hariprasad Govindharajan]

    In current version, we are setting the ports
    using portmask. With portmask, we can use only
    upto 64 ports. This portlist option enables the user
    to use more than 64 ports.
    Now we can specify the ports in 2 different ways
    - Using portmask (-p [0x]nnn): mask must be in hex format
    - Using portlist in the following format
      --portlist <p1>[-p2][,p3[-p4],...]

    --portmask 0x2 is same as --portlist 1
    --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v6:
optimized the code to check for duplicates

v5:
added a check to validate the ports available before
setting them. also added comments in the testpmd file
for the new function

v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values

v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser

v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c                 | 105 +++++++++++++++++++++++++++++++++-
 app/test-pmd/parameters.c             |   5 ++
 app/test-pmd/testpmd.h                |   3 +
 doc/guides/testpmd_app_ug/run_app.rst |   6 ++
 4 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..88144d8 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2564,7 +2564,6 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	unsigned int i;
 	portid_t port_id;
 	int record_now;
-
 	record_now = 0;
  again:
 	for (i = 0; i < nb_pt; i++) {
@@ -2587,6 +2586,110 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+/**
+ * Parse and obtain the list of forwarding ports
+ * from the user input
+ *
+ * @param[in] list
+ *   String containing the user input. User can specify
+ *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ *   For example, if the user wants to use all the available
+ *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ *   If the user wants to use only the ports 1,2 then the input
+ *   is 1,2.
+ *   valid characters are '-' and ','
+ *   invalid chars like '.' or '#' will result in
+ *   EAL: Error - exiting with code: 1
+ *     Cause: Invalid fwd port list
+ * @param[in] values
+ *   This array will be filled with valid ports available in
+ *   the system
+ * @param[in] maxsize
+ *   Size of the values array
+ * @return
+ *   -On success, returns valid count of ports.
+ *   -On failure, returns -1.
+ */
+static int
+parse_port_list(const char *list, unsigned int *values, int maxsize)
+{
+	int count = 0;
+	char *end = NULL;
+	int min, max;
+	int idx;
+	unsigned int freq[RTE_MAX_ETHPORTS] = {0};
+
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	/* Remove all blank characters ahead */
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
+
+	do {
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			return -1;
+		errno = 0;
+		idx = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (idx < 0 || idx >= maxsize)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = idx;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = idx;
+			if (min == maxsize)
+				min = idx;
+			for (idx = min; idx <= max; idx++) {
+				if ((count < maxsize) &&
+					rte_eth_dev_is_valid_port(idx)) {
+					if (freq[idx])
+						continue;
+					values[count] = idx;
+					freq[idx] = 1;
+					count++;
+				}
+			}
+			min = maxsize;
+		} else
+			return -1;
+		list = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+	return count;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	int portcount = 0;
+	unsigned int portindex[RTE_MAX_ETHPORTS];
+
+	/*
+	 * parse_port_list() will mark the portindex array
+	 * with -1 if the port is not listed and with a positive value
+	 * for the listed ports. So, the parser is designed in
+	 * such a way that it will fill the portindex array with the
+	 * valid ports from the user,and the function set_fwd_ports_list()
+	 * will set those ports in the forwarding mode
+	 */
+
+	portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
+	if (portcount < 0)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	set_fwd_ports_list(portindex, portcount);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 9ab4d70..549eacd 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -77,6 +77,12 @@ The command line options are:
 
     Set the hexadecimal bitmask of the ports used by the packet forwarding test.
 
+*   ``--portlist=X``
+
+      Set the forwarding ports based on the user input used by the packet forwarding test.
+      '-' is used for range, inclusive and ',' to provide multiple values.
+      Possible examples like --portlist=0,1 or --portlist=0-2 or --portlist=0,1-2 etc
+
 *   ``--numa``
 
     Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
-- 
2.7.4

[dpdk-dev] [PATCH v7] app/testpmd: add portlist option

[Hariprasad Govindharajan]

In current version, we are setting the ports
using portmask. With portmask, we can use only
upto 64 ports. This portlist option enables the user
to use more than 64 ports.
Now we can specify the ports in 2 different ways
 - Using portmask (-p [0x]nnn): mask must be in hex format
 - Using portlist in the following format
 --portlist <p1>[-p2][,p3[-p4],...]

 --portmask 0x2 is same as --portlist 1
 --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v7:
moved the port validation outside the parser function.
added meaningful comments describing the new functionality.
renamed the variables with meaningful names

v6:
optimized the code to check for duplicates

v5:
added a check to validate the ports available before
setting them. also added comments in the testpmd file
for the new function

v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values

v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser

v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c                 | 114 ++++++++++++++++++++++++++++++++++
 app/test-pmd/parameters.c             |   5 ++
 app/test-pmd/testpmd.h                |   3 +
 doc/guides/testpmd_app_ug/run_app.rst |   7 +++
 4 files changed, 129 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..962984b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+/**
+ * Parse the user input and obtain the list of forwarding ports
+ *
+ * @param[in] list
+ *   String containing the user input. User can specify
+ *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ *   For example, if the user wants to use all the available
+ *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ *   If the user wants to use only the ports 1,2 then the input
+ *   is 1,2.
+ *   valid characters are '-' and ','
+ *   invalid chars like '.' or '#' will result in
+ *   EAL: Error - exiting with code: 1
+ *     Cause: Invalid fwd port list
+ * @param[out] values
+ *   This array will be filled with a list of port IDs
+ *   based on the user input
+ *   Note that duplicate entries are discarded and only the first
+ *   count entries in this array are port IDs and all the rest
+ *   will contain default values
+ * @param[in] maxsize
+ *   This parameter denotes 2 things
+ *   1) Size of the values array
+ *   2) Maximum value of each element in the values array
+ * @return
+ *   -On success, returns total count of port IDs
+ *   -On failure, returns -1.
+ */
+static int
+parse_port_list(const char *list, unsigned int *values, int maxsize)
+{
+	int count = 0;
+	char *end = NULL;
+	int min, max;
+	int value, i;
+	unsigned int marked[maxsize];
+
+	for (i = 0; i < maxsize; i++)
+		marked[i] = 0;
+
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	/* Remove all blank characters ahead */
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
+
+	do {
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			return -1;
+		errno = 0;
+		value = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return -1;
+		if (value < 0 || value >= maxsize)
+			return -1;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = value;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = value;
+			if (min == maxsize)
+				min = value;
+			for (i = min; i <= max; i++) {
+				if (count < maxsize) {
+					if (marked[i])
+						continue;
+					values[count] = i;
+					marked[i] = 1;
+					count++;
+				}
+			}
+			min = maxsize;
+		} else
+			return -1;
+		list = end + 1;
+	} while (*end != '\0');
+
+	if (count == 0)
+		return -1;
+	return count;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	int portcount;
+	unsigned int portindex[RTE_MAX_ETHPORTS];
+	int i, valid_port_count = 0;
+
+	portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
+	if (portcount < 0)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	/*
+	 * Here we verify the validity of the ports
+	 * and thereby calculate the total number of
+	 * valid ports
+	 */
+	for (i = 0; i < portcount && valid_port_count < portcount; i++) {
+		if (rte_eth_dev_is_valid_port(portindex[i])) {
+			portindex[valid_port_count] = portindex[i];
+			valid_port_count++;
+		}
+	}
+
+	set_fwd_ports_list(portindex, valid_port_count);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 9ab4d70..727ef52 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -77,6 +77,13 @@ The command line options are:
 
     Set the hexadecimal bitmask of the ports used by the packet forwarding test.
 
+*   ``--portlist=X``
+
+      Set the forwarding ports based on the user input used by the packet forwarding test.
+      '-' denotes a range of ports to set including the two specified port IDs
+      ',' separates multiple port values.
+      Possible examples like --portlist=0,1 or --portlist=0-2 or --portlist=0,1-2 etc
+
 *   ``--numa``
 
     Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
-- 
2.7.4

Re: [dpdk-dev] [PATCH v7] app/testpmd: add portlist option

[Burakov, Anatoly]

On 10-Feb-20 5:19 PM, Hariprasad Govindharajan wrote:
> In current version, we are setting the ports
> using portmask. With portmask, we can use only
> upto 64 ports. This portlist option enables the user
> to use more than 64 ports.
> Now we can specify the ports in 2 different ways
>   - Using portmask (-p [0x]nnn): mask must be in hex format
>   - Using portlist in the following format
>   --portlist <p1>[-p2][,p3[-p4],...]
> 
>   --portmask 0x2 is same as --portlist 1
>   --portmask 0x3 is same as --portlist 0-1
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
> ---
> v7:
> moved the port validation outside the parser function.
> added meaningful comments describing the new functionality.
> renamed the variables with meaningful names
> 
> v6:
> optimized the code to check for duplicates
> 
> v5:
> added a check to validate the ports available before
> setting them. also added comments in the testpmd file
> for the new function
> 
> v4:
> the parser is modified so that we don't ues 2 arrays
> to convert the listed port values
> 
> v3:
> squashed the 2 patches and made it 1 patch with
> changes only in testpmd. Also working on optmizing
> the parser
> 
> v2:
> moved the parser function to testpmd
> ---
>   app/test-pmd/config.c                 | 114 ++++++++++++++++++++++++++++++++++
>   app/test-pmd/parameters.c             |   5 ++
>   app/test-pmd/testpmd.h                |   3 +
>   doc/guides/testpmd_app_ug/run_app.rst |   7 +++
>   4 files changed, 129 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 9669cbd..962984b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
>   	}
>   }
>   
> +/**
> + * Parse the user input and obtain the list of forwarding ports
> + *
> + * @param[in] list
> + *   String containing the user input. User can specify
> + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + *   For example, if the user wants to use all the available
> + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + *   If the user wants to use only the ports 1,2 then the input
> + *   is 1,2.
> + *   valid characters are '-' and ','
> + *   invalid chars like '.' or '#' will result in
> + *   EAL: Error - exiting with code: 1
> + *     Cause: Invalid fwd port list
> + * @param[out] values
> + *   This array will be filled with a list of port IDs
> + *   based on the user input
> + *   Note that duplicate entries are discarded and only the first
> + *   count entries in this array are port IDs and all the rest
> + *   will contain default values
> + * @param[in] maxsize
> + *   This parameter denotes 2 things
> + *   1) Size of the values array

I believe you meant "number", not "size".

> + *   2) Maximum value of each element in the values array
> + * @return
> + *   -On success, returns total count of port IDs
> + *   -On failure, returns -1.
> + */
> +static int
> +parse_port_list(const char *list, unsigned int *values, int maxsize)
> +{
> +	int count = 0;
> +	char *end = NULL;
> +	int min, max;
> +	int value, i;
> +	unsigned int marked[maxsize];
> +
> +	for (i = 0; i < maxsize; i++)
> +		marked[i] = 0;

Wouldn't marked[maxsize] = {0}; work the same?

> +
> +	if (list == NULL || values == NULL || maxsize < 0)
> +		return -1;

You're checking if maxsize can be negative. First of all, you've already 
allocated the array with negative size by this time (the 
"marked[maxsize]" one), second, why allow negative values at all? Why 
not just make it unsigned?

> +
> +	/* Remove all blank characters ahead */
> +	while (isblank(*list))
> +		list++;

Why do it here when you do this first thing in the do..while loop anyway?

> +
> +	min = maxsize;

You're overwriting this value regardless. Why not 0? If you want to know 
for sure that the value either has or has not been modified, the 
conventional way to do this is to use INT_MAX from <limits.h>.

> +
> +	do {
> +		while (isblank(*list))
> +			list++;

I have a suspicion that isblank() will not return 'true' on '\0' so 
there's probably a buffer overrun here, if you try to dereference *list 
while going past '\0'.

> +		if (*list == '\0')
> +			return -1;
> +		errno = 0;
> +		value = strtol(list, &end, 10);
> +		if (errno || end == NULL)
> +			return -1;
> +		if (value < 0 || value >= maxsize)
> +			return -1;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = value;
> +		} else if ((*end == ',') || (*end == '\0')) {
> +			max = value;
> +			if (min == maxsize)
> +				min = value;
> +			for (i = min; i <= max; i++) {
> +				if (count < maxsize) {
> +					if (marked[i])
> +						continue;
> +					values[count] = i;
> +					marked[i] = 1;
> +					count++;
> +				}
> +			}
> +			min = maxsize;

Probably clearer to reset both to zero or INT_MAX/INT_MIN?

> +		} else
> +			return -1;
> +		list = end + 1;
> +	} while (*end != '\0');
> +
> +	if (count == 0)
> +		return -1;
> +	return count;
> +}
> +
> +void
> +parse_fwd_portlist(const char *portlist)
> +{
> +	int portcount;
> +	unsigned int portindex[RTE_MAX_ETHPORTS];
> +	int i, valid_port_count = 0;

unsigned?

> +
> +	portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
> +	if (portcount < 0)
> +		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
> +
> +	/*
> +	 * Here we verify the validity of the ports
> +	 * and thereby calculate the total number of
> +	 * valid ports
> +	 */

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v7] app/testpmd: add portlist option

[Govindharajan, Hariprasad]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, February 11, 2020 12:01 PM
> To: Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> stephen@networkplumber.org; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v7] app/testpmd: add portlist option
> 
> On 10-Feb-20 5:19 PM, Hariprasad Govindharajan wrote:
> > In current version, we are setting the ports using portmask. With
> > portmask, we can use only upto 64 ports. This portlist option enables
> > the user to use more than 64 ports.
> > Now we can specify the ports in 2 different ways
> >   - Using portmask (-p [0x]nnn): mask must be in hex format
> >   - Using portlist in the following format
> >   --portlist <p1>[-p2][,p3[-p4],...]
> >
> >   --portmask 0x2 is same as --portlist 1
> >   --portmask 0x3 is same as --portlist 0-1
> >
> > Signed-off-by: Hariprasad Govindharajan
> > <hariprasad.govindharajan@intel.com>
> > ---
> > v7:
> > moved the port validation outside the parser function.
> > added meaningful comments describing the new functionality.
> > renamed the variables with meaningful names
> >
> > v6:
> > optimized the code to check for duplicates
> >
> > v5:
> > added a check to validate the ports available before setting them.
> > also added comments in the testpmd file for the new function
> >
> > v4:
> > the parser is modified so that we don't ues 2 arrays to convert the
> > listed port values
> >
> > v3:
> > squashed the 2 patches and made it 1 patch with changes only in
> > testpmd. Also working on optmizing the parser
> >
> > v2:
> > moved the parser function to testpmd
> > ---
> >   app/test-pmd/config.c                 | 114
> ++++++++++++++++++++++++++++++++++
> >   app/test-pmd/parameters.c             |   5 ++
> >   app/test-pmd/testpmd.h                |   3 +
> >   doc/guides/testpmd_app_ug/run_app.rst |   7 +++
> >   4 files changed, 129 insertions(+)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 9669cbd..962984b 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -2587,6 +2587,120 @@ set_fwd_ports_list(unsigned int *portlist,
> unsigned int nb_pt)
> >   	}
> >   }
> >
> > +/**
> > + * Parse the user input and obtain the list of forwarding ports
> > + *
> > + * @param[in] list
> > + *   String containing the user input. User can specify
> > + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> > + *   For example, if the user wants to use all the available
> > + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> > + *   If the user wants to use only the ports 1,2 then the input
> > + *   is 1,2.
> > + *   valid characters are '-' and ','
> > + *   invalid chars like '.' or '#' will result in
> > + *   EAL: Error - exiting with code: 1
> > + *     Cause: Invalid fwd port list
> > + * @param[out] values
> > + *   This array will be filled with a list of port IDs
> > + *   based on the user input
> > + *   Note that duplicate entries are discarded and only the first
> > + *   count entries in this array are port IDs and all the rest
> > + *   will contain default values
> > + * @param[in] maxsize
> > + *   This parameter denotes 2 things
> > + *   1) Size of the values array
> 
> I believe you meant "number", not "size".

[Govindharajan, Hariprasad] Nope. Here I meant to say the maximum size of the values array.
  
> 
> > + *   2) Maximum value of each element in the values array
> > + * @return
> > + *   -On success, returns total count of port IDs
> > + *   -On failure, returns -1.
> > + */
> > +static int
> > +parse_port_list(const char *list, unsigned int *values, int maxsize)
> > +{
> > +	int count = 0;
> > +	char *end = NULL;
> > +	int min, max;
> > +	int value, i;
> > +	unsigned int marked[maxsize];
> > +
> > +	for (i = 0; i < maxsize; i++)
> > +		marked[i] = 0;
> 
> Wouldn't marked[maxsize] = {0}; work the same?

[Govindharajan, Hariprasad] Nope. For that to work, the array size should be a constant. Here it is a variable.
> 
> > +
> > +	if (list == NULL || values == NULL || maxsize < 0)
> > +		return -1;
> 
> You're checking if maxsize can be negative. First of all, you've already
> allocated the array with negative size by this time (the "marked[maxsize]"
> one), second, why allow negative values at all? Why not just make it
> unsigned?
> 
> > +
> > +	/* Remove all blank characters ahead */
> > +	while (isblank(*list))
> > +		list++;
> 
> Why do it here when you do this first thing in the do..while loop anyway?

[Govindharajan, Hariprasad] Yes. Removed.
> 
> > +
> > +	min = maxsize;
> 
> You're overwriting this value regardless. Why not 0? If you want to know for
> sure that the value either has or has not been modified, the conventional
> way to do this is to use INT_MAX from <limits.h>.
> 
> > +
> > +	do {
> > +		while (isblank(*list))
> > +			list++;
> 
> I have a suspicion that isblank() will not return 'true' on '\0' so there's
> probably a buffer overrun here, if you try to dereference *list while going
> past '\0'.

[Govindharajan, Hariprasad]  Corrected
> 
> > +		if (*list == '\0')
> > +			return -1;
> > +		errno = 0;
> > +		value = strtol(list, &end, 10);
> > +		if (errno || end == NULL)
> > +			return -1;
> > +		if (value < 0 || value >= maxsize)
> > +			return -1;
> > +		while (isblank(*end))
> > +			end++;
> > +		if (*end == '-') {
> > +			min = value;
> > +		} else if ((*end == ',') || (*end == '\0')) {
> > +			max = value;
> > +			if (min == maxsize)
> > +				min = value;
> > +			for (i = min; i <= max; i++) {
> > +				if (count < maxsize) {
> > +					if (marked[i])
> > +						continue;
> > +					values[count] = i;
> > +					marked[i] = 1;
> > +					count++;
> > +				}
> > +			}
> > +			min = maxsize;
> 
> Probably clearer to reset both to zero or INT_MAX/INT_MIN?

[Govindharajan, Hariprasad]  done
> 
> > +		} else
> > +			return -1;
> > +		list = end + 1;
> > +	} while (*end != '\0');
> > +
> > +	if (count == 0)
> > +		return -1;
> > +	return count;
> > +}
> > +
> > +void
> > +parse_fwd_portlist(const char *portlist) {
> > +	int portcount;
> > +	unsigned int portindex[RTE_MAX_ETHPORTS];
> > +	int i, valid_port_count = 0;
> 
> unsigned?

[Govindharajan, Hariprasad] Changed. Initially I was comparing those 2 variables with a signed variable,
So declared them as signed as well.
> 
> > +
> > +	portcount = parse_port_list(portlist, portindex,
> RTE_MAX_ETHPORTS);
> > +	if (portcount < 0)
> > +		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
> > +
> > +	/*
> > +	 * Here we verify the validity of the ports
> > +	 * and thereby calculate the total number of
> > +	 * valid ports
> > +	 */
> 
> --
> Thanks,
> Anatoly

[dpdk-dev] [PATCH v8] app/testpmd: add portlist option

[Hariprasad Govindharajan]

In current version, we are setting the ports
using portmask. With portmask, we can use only
upto 64 ports. This portlist option enables the user
to use more than 64 ports.
Now we can specify the ports in 2 different ways
 - Using portmask (-p [0x]nnn): mask must be in hex format
 - Using portlist in the following format
 --portlist <p1>[-p2][,p3[-p4],...]

 --portmask 0x2 is same as --portlist 1
 --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v8:
changed the data types of the variables.
optimised the code by checking for blank spaces
only once.

v7:
moved the port validation outside the parser function.
added meaningful comments describing the new functionality.
renamed the variables with meaningful names

v6:
optimized the code to check for duplicates

v5:
added a check to validate the ports available before
setting them. also added comments in the testpmd file
for the new function

v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values

v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser

v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c                 | 108 ++++++++++++++++++++++++++++++++++
 app/test-pmd/parameters.c             |   5 ++
 app/test-pmd/testpmd.h                |   3 +
 doc/guides/testpmd_app_ug/run_app.rst |   7 +++
 4 files changed, 123 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..86566d9 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2587,6 +2587,114 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+/**
+ * Parse the user input and obtain the list of forwarding ports
+ *
+ * @param[in] list
+ *   String containing the user input. User can specify
+ *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ *   For example, if the user wants to use all the available
+ *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ *   If the user wants to use only the ports 1,2 then the input
+ *   is 1,2.
+ *   valid characters are '-' and ','
+ *   invalid chars like '.' or '#' will result in
+ *   EAL: Error - exiting with code: 1
+ *     Cause: Invalid fwd port list
+ * @param[out] values
+ *   This array will be filled with a list of port IDs
+ *   based on the user input
+ *   Note that duplicate entries are discarded and only the first
+ *   count entries in this array are port IDs and all the rest
+ *   will contain default values
+ * @param[in] maxsize
+ *   This parameter denotes 2 things
+ *   1) Maximum size of the values array
+ *   2) Maximum value of each element in the values array
+ * @return
+ *   -returns total count of parsed port IDs
+ */
+static unsigned int
+parse_port_list(const char *list, unsigned int *values, unsigned int maxsize)
+{
+	unsigned int count = 0;
+	char *end = NULL;
+	int min, max;
+	int value, i;
+	unsigned int marked[maxsize];
+
+	if (list == NULL || values == NULL)
+		return -1;
+
+	for (i = 0; i < (int)maxsize; i++)
+		marked[i] = 0;
+
+	min = INT_MAX;
+
+	do {
+		/*Remove the blank spaces if any*/
+		while (*list != '\0' && isblank(*list))
+			list++;
+		if (*list == '\0')
+			break;
+		errno = 0;
+		value = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return 0;
+		if (value < 0 || value >= (int)maxsize)
+			return 0;
+		while (isblank(*end))
+			end++;
+		if (*end == '-') {
+			min = value;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = value;
+			if (min == INT_MAX)
+				min = value;
+			for (i = min; i <= max; i++) {
+				if (count < maxsize) {
+					if (marked[i])
+						continue;
+					values[count] = i;
+					marked[i] = 1;
+					count++;
+				}
+			}
+			min = INT_MAX;
+		} else
+			return 0;
+		list = end + 1;
+	} while (*end != '\0');
+
+	return count;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	unsigned int portcount;
+	unsigned int portindex[RTE_MAX_ETHPORTS];
+	unsigned int i, valid_port_count = 0;
+
+	portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
+	if (!portcount)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	/*
+	 * Here we verify the validity of the ports
+	 * and thereby calculate the total number of
+	 * valid ports
+	 */
+	for (i = 0; i < portcount && valid_port_count < portcount; i++) {
+		if (rte_eth_dev_is_valid_port(portindex[i])) {
+			portindex[valid_port_count] = portindex[i];
+			valid_port_count++;
+		}
+	}
+
+	set_fwd_ports_list(portindex, valid_port_count);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 9ab4d70..727ef52 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -77,6 +77,13 @@ The command line options are:
 
     Set the hexadecimal bitmask of the ports used by the packet forwarding test.
 
+*   ``--portlist=X``
+
+      Set the forwarding ports based on the user input used by the packet forwarding test.
+      '-' denotes a range of ports to set including the two specified port IDs
+      ',' separates multiple port values.
+      Possible examples like --portlist=0,1 or --portlist=0-2 or --portlist=0,1-2 etc
+
 *   ``--numa``
 
     Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
-- 
2.7.4

Re: [dpdk-dev] [PATCH v8] app/testpmd: add portlist option

[Burakov, Anatoly]

On 11-Feb-20 3:52 PM, Hariprasad Govindharajan wrote:
> In current version, we are setting the ports
> using portmask. With portmask, we can use only
> upto 64 ports. This portlist option enables the user
> to use more than 64 ports.
> Now we can specify the ports in 2 different ways
>   - Using portmask (-p [0x]nnn): mask must be in hex format
>   - Using portlist in the following format
>   --portlist <p1>[-p2][,p3[-p4],...]
> 
>   --portmask 0x2 is same as --portlist 1
>   --portmask 0x3 is same as --portlist 0-1
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
> ---
> v8:
> changed the data types of the variables.
> optimised the code by checking for blank spaces
> only once.
> 
> v7:
> moved the port validation outside the parser function.
> added meaningful comments describing the new functionality.
> renamed the variables with meaningful names
> 
> v6:
> optimized the code to check for duplicates
> 
> v5:
> added a check to validate the ports available before
> setting them. also added comments in the testpmd file
> for the new function
> 
> v4:
> the parser is modified so that we don't ues 2 arrays
> to convert the listed port values
> 
> v3:
> squashed the 2 patches and made it 1 patch with
> changes only in testpmd. Also working on optmizing
> the parser
> 
> v2:
> moved the parser function to testpmd
> ---
>   app/test-pmd/config.c                 | 108 ++++++++++++++++++++++++++++++++++
>   app/test-pmd/parameters.c             |   5 ++
>   app/test-pmd/testpmd.h                |   3 +
>   doc/guides/testpmd_app_ug/run_app.rst |   7 +++
>   4 files changed, 123 insertions(+)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 9669cbd..86566d9 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2587,6 +2587,114 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
>   	}
>   }
>   
> +/**
> + * Parse the user input and obtain the list of forwarding ports
> + *
> + * @param[in] list
> + *   String containing the user input. User can specify
> + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + *   For example, if the user wants to use all the available
> + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + *   If the user wants to use only the ports 1,2 then the input
> + *   is 1,2.
> + *   valid characters are '-' and ','
> + *   invalid chars like '.' or '#' will result in
> + *   EAL: Error - exiting with code: 1
> + *     Cause: Invalid fwd port list
> + * @param[out] values
> + *   This array will be filled with a list of port IDs
> + *   based on the user input
> + *   Note that duplicate entries are discarded and only the first
> + *   count entries in this array are port IDs and all the rest
> + *   will contain default values
> + * @param[in] maxsize
> + *   This parameter denotes 2 things
> + *   1) Maximum size of the values array
> + *   2) Maximum value of each element in the values array

I still suspect the first item should say "number", not size. The 2) 
takes care of how big each individual value is, and 1) presumably takes 
care of how many of these values there can be. Therefore i think it 
should be "number" (as in how many), not "size" (as in how big).

> + * @return
> + *   -returns total count of parsed port IDs
> + */
> +static unsigned int
> +parse_port_list(const char *list, unsigned int *values, unsigned int maxsize)
> +{
> +	unsigned int count = 0;
> +	char *end = NULL;
> +	int min, max;
> +	int value, i;
> +	unsigned int marked[maxsize];
> +
> +	if (list == NULL || values == NULL)
> +		return -1;
> +
> +	for (i = 0; i < (int)maxsize; i++)
> +		marked[i] = 0;

Then memset(), but that's just nitpicking, so feel free to disregard :)

> +
> +	min = INT_MAX;
> +
> +	do {
> +		/*Remove the blank spaces if any*/
> +		while (*list != '\0' && isblank(*list))
> +			list++;

My apologies. I've just checked if isblank() returns 0 on '\0', and it 
does. So, the `*list != '\0'` check is not necessary here after all.

> +		if (*list == '\0')
> +			break;
> +		errno = 0;
> +		value = strtol(list, &end, 10);
> +		if (errno || end == NULL)
> +			return 0;
> +		if (value < 0 || value >= (int)maxsize)
> +			return 0;
> +		while (isblank(*end))
> +			end++;
> +		if (*end == '-') {
> +			min = value;

This would accept input such as "1-2-3" and parse it as "2-3". Maybe

if (*end == '-' && min == INT_MAX)

? This would then fall through to the failure path if end was '-' and 
min was already set.


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v8] app/testpmd: add portlist option

[Govindharajan, Hariprasad]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, February 11, 2020 4:52 PM
> To: Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>;
> stephen@networkplumber.org; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v8] app/testpmd: add portlist option
> 
> On 11-Feb-20 3:52 PM, Hariprasad Govindharajan wrote:
> > In current version, we are setting the ports using portmask. With
> > portmask, we can use only upto 64 ports. This portlist option enables
> > the user to use more than 64 ports.
> > Now we can specify the ports in 2 different ways
> >   - Using portmask (-p [0x]nnn): mask must be in hex format
> >   - Using portlist in the following format
> >   --portlist <p1>[-p2][,p3[-p4],...]
> >
> >   --portmask 0x2 is same as --portlist 1
> >   --portmask 0x3 is same as --portlist 0-1
> >
> > Signed-off-by: Hariprasad Govindharajan
> > <hariprasad.govindharajan@intel.com>
> > ---
> > v8:
> > changed the data types of the variables.
> > optimised the code by checking for blank spaces only once.
> >
> > v7:
> > moved the port validation outside the parser function.
> > added meaningful comments describing the new functionality.
> > renamed the variables with meaningful names
> >
> > v6:
> > optimized the code to check for duplicates
> >
> > v5:
> > added a check to validate the ports available before setting them.
> > also added comments in the testpmd file for the new function
> >
> > v4:
> > the parser is modified so that we don't ues 2 arrays to convert the
> > listed port values
> >
> > v3:
> > squashed the 2 patches and made it 1 patch with changes only in
> > testpmd. Also working on optmizing the parser
> >
> > v2:
> > moved the parser function to testpmd
> > ---
> >   app/test-pmd/config.c                 | 108
> ++++++++++++++++++++++++++++++++++
> >   app/test-pmd/parameters.c             |   5 ++
> >   app/test-pmd/testpmd.h                |   3 +
> >   doc/guides/testpmd_app_ug/run_app.rst |   7 +++
> >   4 files changed, 123 insertions(+)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 9669cbd..86566d9 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -2587,6 +2587,114 @@ set_fwd_ports_list(unsigned int *portlist,
> unsigned int nb_pt)
> >   	}
> >   }
> >
> > +/**
> > + * Parse the user input and obtain the list of forwarding ports
> > + *
> > + * @param[in] list
> > + *   String containing the user input. User can specify
> > + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> > + *   For example, if the user wants to use all the available
> > + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> > + *   If the user wants to use only the ports 1,2 then the input
> > + *   is 1,2.
> > + *   valid characters are '-' and ','
> > + *   invalid chars like '.' or '#' will result in
> > + *   EAL: Error - exiting with code: 1
> > + *     Cause: Invalid fwd port list
> > + * @param[out] values
> > + *   This array will be filled with a list of port IDs
> > + *   based on the user input
> > + *   Note that duplicate entries are discarded and only the first
> > + *   count entries in this array are port IDs and all the rest
> > + *   will contain default values
> > + * @param[in] maxsize
> > + *   This parameter denotes 2 things
> > + *   1) Maximum size of the values array
> > + *   2) Maximum value of each element in the values array
> 
> I still suspect the first item should say "number", not size. The 2) takes care of
> how big each individual value is, and 1) presumably takes care of how many
> of these values there can be. Therefore i think it should be "number" (as in
> how many), not "size" (as in how big).
> 

[Govindharajan, Hariprasad]  Changed
> > + * @return
> > + *   -returns total count of parsed port IDs
> > + */
> > +static unsigned int
> > +parse_port_list(const char *list, unsigned int *values, unsigned int
> > +maxsize) {
> > +	unsigned int count = 0;
> > +	char *end = NULL;
> > +	int min, max;
> > +	int value, i;
> > +	unsigned int marked[maxsize];
> > +
> > +	if (list == NULL || values == NULL)
> > +		return -1;
> > +
> > +	for (i = 0; i < (int)maxsize; i++)
> > +		marked[i] = 0;
> 
> Then memset(), but that's just nitpicking, so feel free to disregard :)

[Govindharajan, Hariprasad] It is disregarded.. 😊
> 
> > +
> > +	min = INT_MAX;
> > +
> > +	do {
> > +		/*Remove the blank spaces if any*/
> > +		while (*list != '\0' && isblank(*list))
> > +			list++;
> 
> My apologies. I've just checked if isblank() returns 0 on '\0', and it does. So,
> the `*list != '\0'` check is not necessary here after all.

[Govindharajan, Hariprasad] Removed the isblank check
> 
> > +		if (*list == '\0')
> > +			break;
> > +		errno = 0;
> > +		value = strtol(list, &end, 10);
> > +		if (errno || end == NULL)
> > +			return 0;
> > +		if (value < 0 || value >= (int)maxsize)
> > +			return 0;
> > +		while (isblank(*end))
> > +			end++;
> > +		if (*end == '-') {
> > +			min = value;
> 
> This would accept input such as "1-2-3" and parse it as "2-3". Maybe
> 
> if (*end == '-' && min == INT_MAX)
> 
> ? This would then fall through to the failure path if end was '-' and min was
> already set.
>
[Govindharajan, Hariprasad] corrected.
> 
> --
> Thanks,
> Anatoly

[dpdk-dev] [PATCH v9] app/testpmd: add portlist option

[Hariprasad Govindharajan]

In current version, we are setting the ports
using portmask. With portmask, we can use only
upto 64 ports. This portlist option enables the user
to use more than 64 ports.
Now we can specify the ports in 2 different ways
 - Using portmask (-p [0x]nnn): mask must be in hex format
 - Using portlist in the following format
 --portlist <p1>[-p2][,p3[-p4],...]

 --portmask 0x2 is same as --portlist 1
 --portmask 0x3 is same as --portlist 0-1

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
v9:
removed the isblank() check.
updated the comments.
fixed a bug which takes the range correctly.

v8:
changed the data types of the variables.
optimised the code by checking for blank spaces
only once.

v7:
moved the port validation outside the parser function.
added meaningful comments describing the new functionality.
renamed the variables with meaningful names

v6:
optimized the code to check for duplicates

v5:
added a check to validate the ports available before
setting them. also added comments in the testpmd file
for the new function

v4:
the parser is modified so that we don't ues 2 arrays
to convert the listed port values

v3:
squashed the 2 patches and made it 1 patch with
changes only in testpmd. Also working on optmizing
the parser

v2:
moved the parser function to testpmd
---
 app/test-pmd/config.c                 | 108 ++++++++++++++++++++++++++++++++++
 app/test-pmd/parameters.c             |   5 ++
 app/test-pmd/testpmd.h                |   3 +
 doc/guides/testpmd_app_ug/run_app.rst |   7 +++
 4 files changed, 123 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 9669cbd..d8649f1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2587,6 +2587,114 @@ set_fwd_ports_list(unsigned int *portlist, unsigned int nb_pt)
 	}
 }
 
+/**
+ * Parse the user input and obtain the list of forwarding ports
+ *
+ * @param[in] list
+ *   String containing the user input. User can specify
+ *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ *   For example, if the user wants to use all the available
+ *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ *   If the user wants to use only the ports 1,2 then the input
+ *   is 1,2.
+ *   valid characters are '-' and ','
+ *   invalid chars like '.' or '#' will result in
+ *   EAL: Error - exiting with code: 1
+ *     Cause: Invalid fwd port list
+ * @param[out] values
+ *   This array will be filled with a list of port IDs
+ *   based on the user input
+ *   Note that duplicate entries are discarded and only the first
+ *   count entries in this array are port IDs and all the rest
+ *   will contain default values
+ * @param[in] maxsize
+ *   This parameter denotes 2 things
+ *   1) Number of elements in the values array
+ *   2) Maximum value of each element in the values array
+ * @return
+ *   -returns total count of parsed port IDs
+ */
+static unsigned int
+parse_port_list(const char *list, unsigned int *values, unsigned int maxsize)
+{
+	unsigned int count = 0;
+	char *end = NULL;
+	int min, max;
+	int value, i;
+	unsigned int marked[maxsize];
+
+	if (list == NULL || values == NULL)
+		return -1;
+
+	for (i = 0; i < (int)maxsize; i++)
+		marked[i] = 0;
+
+	min = INT_MAX;
+
+	do {
+		/*Remove the blank spaces if any*/
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
+			break;
+		errno = 0;
+		value = strtol(list, &end, 10);
+		if (errno || end == NULL)
+			return 0;
+		if (value < 0 || value >= (int)maxsize)
+			return 0;
+		while (isblank(*end))
+			end++;
+		if (*end == '-' && min == INT_MAX) {
+			min = value;
+		} else if ((*end == ',') || (*end == '\0')) {
+			max = value;
+			if (min == INT_MAX)
+				min = value;
+			for (i = min; i <= max; i++) {
+				if (count < maxsize) {
+					if (marked[i])
+						continue;
+					values[count] = i;
+					marked[i] = 1;
+					count++;
+				}
+			}
+			min = INT_MAX;
+		} else
+			return 0;
+		list = end + 1;
+	} while (*end != '\0');
+
+	return count;
+}
+
+void
+parse_fwd_portlist(const char *portlist)
+{
+	unsigned int portcount;
+	unsigned int portindex[RTE_MAX_ETHPORTS];
+	unsigned int i, valid_port_count = 0;
+
+	portcount = parse_port_list(portlist, portindex, RTE_MAX_ETHPORTS);
+	if (!portcount)
+		rte_exit(EXIT_FAILURE, "Invalid fwd port list\n");
+
+	/*
+	 * Here we verify the validity of the ports
+	 * and thereby calculate the total number of
+	 * valid ports
+	 */
+	for (i = 0; i < portcount && valid_port_count < portcount; i++) {
+		if (rte_eth_dev_is_valid_port(portindex[i])) {
+			portindex[valid_port_count] = portindex[i];
+			valid_port_count++;
+		}
+	}
+
+	set_fwd_ports_list(portindex, valid_port_count);
+}
+
 void
 set_fwd_ports_mask(uint64_t portmask)
 {
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 6340104..404dba2 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -57,6 +57,7 @@ usage(char* progname)
 	       "[--help|-h] | [--auto-start|-a] | ["
 	       "--tx-first | --stats-period=PERIOD | "
 	       "--coremask=COREMASK --portmask=PORTMASK --numa "
+	       "--portlist=PORTLIST "
 	       "--mbuf-size= | --total-num-mbufs= | "
 	       "--nb-cores= | --nb-ports= | "
 #ifdef RTE_LIBRTE_CMDLINE
@@ -92,6 +93,7 @@ usage(char* progname)
 	       "packet forwarding.\n");
 	printf("  --portmask=PORTMASK: hexadecimal bitmask of ports used "
 	       "by the packet forwarding test.\n");
+	printf("  --portlist=PORTLIST: list of forwarding ports\n");
 	printf("  --numa: enable NUMA-aware allocation of RX/TX rings and of "
 	       "RX memory buffers (mbufs).\n");
 	printf("  --port-numa-config=(port,socket)[,(port,socket)]: "
@@ -587,6 +589,7 @@ launch_args_parse(int argc, char** argv)
 		{ "nb-ports",			1, 0, 0 },
 		{ "coremask",			1, 0, 0 },
 		{ "portmask",			1, 0, 0 },
+		{ "portlist",			1, 0, 0 },
 		{ "numa",			0, 0, 0 },
 		{ "no-numa",			0, 0, 0 },
 		{ "mp-anon",			0, 0, 0 },
@@ -825,6 +828,8 @@ launch_args_parse(int argc, char** argv)
 				parse_fwd_coremask(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "portmask"))
 				parse_fwd_portmask(optarg);
+			if (!strcmp(lgopts[opt_idx].name, "portlist"))
+				parse_fwd_portlist(optarg);
 			if (!strcmp(lgopts[opt_idx].name, "no-numa"))
 				numa_support = 0;
 			if (!strcmp(lgopts[opt_idx].name, "numa"))
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3dd5fc7..33ef3e2 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -614,6 +614,9 @@ lcore_num(void)
 	rte_panic("lcore_id of current thread not found in fwd_lcores_cpuids\n");
 }
 
+void
+parse_fwd_portlist(const char *port);
+
 static inline struct fwd_lcore *
 current_fwd_lcore(void)
 {
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 9ab4d70..727ef52 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -77,6 +77,13 @@ The command line options are:
 
     Set the hexadecimal bitmask of the ports used by the packet forwarding test.
 
+*   ``--portlist=X``
+
+      Set the forwarding ports based on the user input used by the packet forwarding test.
+      '-' denotes a range of ports to set including the two specified port IDs
+      ',' separates multiple port values.
+      Possible examples like --portlist=0,1 or --portlist=0-2 or --portlist=0,1-2 etc
+
 *   ``--numa``
 
     Enable NUMA-aware allocation of RX/TX rings and of RX memory buffers
-- 
2.7.4

Re: [dpdk-dev] [PATCH v9] app/testpmd: add portlist option

[Burakov, Anatoly]

On 12-Feb-20 10:29 AM, Hariprasad Govindharajan wrote:
> In current version, we are setting the ports
> using portmask. With portmask, we can use only
> upto 64 ports. This portlist option enables the user
> to use more than 64 ports.
> Now we can specify the ports in 2 different ways
>   - Using portmask (-p [0x]nnn): mask must be in hex format
>   - Using portlist in the following format
>   --portlist <p1>[-p2][,p3[-p4],...]
> 
>   --portmask 0x2 is same as --portlist 1
>   --portmask 0x3 is same as --portlist 0-1
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v9] app/testpmd: add portlist option

[Ferruh Yigit]

On 2/12/2020 1:03 PM, Burakov, Anatoly wrote:
> On 12-Feb-20 10:29 AM, Hariprasad Govindharajan wrote:
>> In current version, we are setting the ports
>> using portmask. With portmask, we can use only
>> upto 64 ports. This portlist option enables the user
>> to use more than 64 ports.
>> Now we can specify the ports in 2 different ways
>>   - Using portmask (-p [0x]nnn): mask must be in hex format
>>   - Using portlist in the following format
>>   --portlist <p1>[-p2][,p3[-p4],...]
>>
>>   --portmask 0x2 is same as --portlist 1
>>   --portmask 0x3 is same as --portlist 0-1
>>
>> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
>> ---
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v9] app/testpmd: add portlist option

[Lipiec, Herakliusz]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> Sent: Wednesday, February 12, 2020 2:00 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; Govindharajan,
> Hariprasad <hariprasad.govindharajan@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger,
> Bernard <bernard.iremonger@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>; Kovacevic, Marko
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org;
> david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v9] app/testpmd: add portlist option
> 
> On 2/12/2020 1:03 PM, Burakov, Anatoly wrote:
> > On 12-Feb-20 10:29 AM, Hariprasad Govindharajan wrote:
> >> In current version, we are setting the ports using portmask. With
> >> portmask, we can use only upto 64 ports. This portlist option enables
> >> the user to use more than 64 ports.
> >> Now we can specify the ports in 2 different ways
> >>   - Using portmask (-p [0x]nnn): mask must be in hex format
> >>   - Using portlist in the following format
> >>   --portlist <p1>[-p2][,p3[-p4],...]
> >>
> >>   --portmask 0x2 is same as --portlist 1
> >>   --portmask 0x3 is same as --portlist 0-1
> >>
> >> Signed-off-by: Hariprasad Govindharajan
> >> <hariprasad.govindharajan@intel.com>
> >> ---
> >
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Reviewed-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>

Re: [dpdk-dev] [PATCH v9] app/testpmd: add portlist option

[Ferruh Yigit]

On 2/12/2020 5:27 PM, Lipiec, Herakliusz wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
>> Sent: Wednesday, February 12, 2020 2:00 PM
>> To: Burakov, Anatoly <anatoly.burakov@intel.com>; Govindharajan,
>> Hariprasad <hariprasad.govindharajan@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Iremonger,
>> Bernard <bernard.iremonger@intel.com>; Mcnamara, John
>> <john.mcnamara@intel.com>; Kovacevic, Marko
>> <marko.kovacevic@intel.com>
>> Cc: dev@dpdk.org; stephen@networkplumber.org;
>> david.marchand@redhat.com
>> Subject: Re: [dpdk-dev] [PATCH v9] app/testpmd: add portlist option
>>
>> On 2/12/2020 1:03 PM, Burakov, Anatoly wrote:
>>> On 12-Feb-20 10:29 AM, Hariprasad Govindharajan wrote:
>>>> In current version, we are setting the ports using portmask. With
>>>> portmask, we can use only upto 64 ports. This portlist option enables
>>>> the user to use more than 64 ports.
>>>> Now we can specify the ports in 2 different ways
>>>>   - Using portmask (-p [0x]nnn): mask must be in hex format
>>>>   - Using portlist in the following format
>>>>   --portlist <p1>[-p2][,p3[-p4],...]
>>>>
>>>>   --portmask 0x2 is same as --portlist 1
>>>>   --portmask 0x3 is same as --portlist 0-1
>>>>
>>>> Signed-off-by: Hariprasad Govindharajan
>>>> <hariprasad.govindharajan@intel.com>
>>>> ---
>>>
>>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>
>>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Reviewed-by: Herakliusz Lipiec <herakliusz.lipiec@intel.com>
> 

Applied to dpdk-next-net/master, thanks.

[dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[Hariprasad Govindharajan]

In current version, there is a function which parses
the corelist based on user value. A new generic
function eal_parse_optionlist is added which will
parse corelist as well as similar user input so
that we can use it as a public API too.

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 45 ++++++++++++++++++------------
 lib/librte_eal/common/include/rte_eal.h    | 34 ++++++++++++++++++++++
 lib/librte_eal/rte_eal_version.map         |  1 +
 3 files changed, 62 insertions(+), 18 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 5920233..aa59f8b 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -571,33 +571,36 @@ eal_parse_service_corelist(const char *corelist)
 	return 0;
 }
 
-static int
-eal_parse_corelist(const char *corelist, int *cores)
+int
+eal_parse_optionlist(const char *list, int *values, int maxsize)
 {
 	unsigned count = 0;
 	char *end = NULL;
 	int min, max;
 	int idx;
 
-	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
-		cores[idx] = -1;
+	if (list == NULL || values == NULL || maxsize < 0)
+		return -1;
+
+	for (idx = 0; idx < maxsize; idx++)
+		values[idx] = -1;
 
 	/* Remove all blank characters ahead */
-	while (isblank(*corelist))
-		corelist++;
+	while (isblank(*list))
+		list++;
+
+	min = maxsize;
 
-	/* Get list of cores */
-	min = RTE_MAX_LCORE;
 	do {
-		while (isblank(*corelist))
-			corelist++;
-		if (*corelist == '\0')
+		while (isblank(*list))
+			list++;
+		if (*list == '\0')
 			return -1;
 		errno = 0;
-		idx = strtol(corelist, &end, 10);
+		idx = strtol(list, &end, 10);
 		if (errno || end == NULL)
 			return -1;
-		if (idx < 0 || idx >= RTE_MAX_LCORE)
+		if (idx < 0 || idx >= maxsize)
 			return -1;
 		while (isblank(*end))
 			end++;
@@ -605,18 +608,18 @@ eal_parse_corelist(const char *corelist, int *cores)
 			min = idx;
 		} else if ((*end == ',') || (*end == '\0')) {
 			max = idx;
-			if (min == RTE_MAX_LCORE)
+			if (min == maxsize)
 				min = idx;
 			for (idx = min; idx <= max; idx++) {
-				if (cores[idx] == -1) {
-					cores[idx] = count;
+				if (values[idx] == -1) {
+					values[idx] = count;
 					count++;
 				}
 			}
-			min = RTE_MAX_LCORE;
+			min = maxsize;
 		} else
 			return -1;
-		corelist = end + 1;
+		list = end + 1;
 	} while (*end != '\0');
 
 	if (count == 0)
@@ -624,6 +627,12 @@ eal_parse_corelist(const char *corelist, int *cores)
 	return 0;
 }
 
+static int
+eal_parse_corelist(const char *corelist, int *cores)
+{
+	return eal_parse_optionlist(corelist, cores, RTE_MAX_LCORE);
+}
+
 /* Changes the lcore id of the master thread */
 static int
 eal_parse_master_lcore(const char *arg)
diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
index 2f9ed29..567b754 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -71,6 +71,40 @@ enum rte_proc_type_t rte_eal_process_type(void);
 int rte_eal_iopl_init(void);
 
 /**
+ * Parse the user input
+ *
+ * This function can be used to read and parse the user input
+ * from the command line. For example, when the user specifies
+ * corelist or port list this function will read the input
+ * and set the forwarding cores or ports
+ *
+ * @param[in] list
+ *   String containing the user input. User can specify
+ *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
+ *   For example, if the user wants to use all the available
+ *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
+ *   If the user wants to use only the ports 1,2 then the input
+ *   is 1,2.
+ *   valid characters are '-' and ','
+ *   invalid chars like '.' or '#' will result in
+ *   EAL: Error - exiting with code: 1
+ *     Cause: Invalid fwd port list
+ * @param[in] values
+ *   An array pointer, used by this function to set the
+ *   array contents to a positive value if they are listed
+ *   in the input
+ *   else sets it to -1
+ * @param[in] maxsize
+ *   This is the maximum value the list string can contain
+ * @return
+ *   -On success, returns 0.
+ *   -On failure, returns -1.
+ */
+__rte_experimental
+int
+eal_parse_optionlist(const char *list, int *values, int maxsize);
+
+/**
  * Initialize the Environment Abstraction Layer (EAL).
  *
  * This function is to be executed on the MASTER lcore only, as soon
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index e38d025..3d72df8 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -332,4 +332,5 @@ EXPERIMENTAL {
 	# added in 19.11
 	rte_log_get_stream;
 	rte_mcfg_get_single_file_segments;
+	eal_parse_optionlist;
 };
-- 
2.7.4

Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[Ferruh Yigit]

On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
> In current version, there is a function which parses
> the corelist based on user value. A new generic
> function eal_parse_optionlist is added which will
> parse corelist as well as similar user input so
> that we can use it as a public API too.
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>

Hi David,

Overall this patchset is to add '--portlist' command to testpmd and remove
existing 64 port limitation.

And in this patch re-uses the exiting parser function in eal and converts it to
API, question is if eal is good place to have this API, what do you think about it?

> ---
>  lib/librte_eal/common/eal_common_options.c | 45 ++++++++++++++++++------------
>  lib/librte_eal/common/include/rte_eal.h    | 34 ++++++++++++++++++++++
>  lib/librte_eal/rte_eal_version.map         |  1 +
>  3 files changed, 62 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 5920233..aa59f8b 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -571,33 +571,36 @@ eal_parse_service_corelist(const char *corelist)
>  	return 0;
>  }
>  
> -static int
> -eal_parse_corelist(const char *corelist, int *cores)
> +int
> +eal_parse_optionlist(const char *list, int *values, int maxsize)
>  {
>  	unsigned count = 0;
>  	char *end = NULL;
>  	int min, max;
>  	int idx;
>  
> -	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
> -		cores[idx] = -1;
> +	if (list == NULL || values == NULL || maxsize < 0)
> +		return -1;
> +
> +	for (idx = 0; idx < maxsize; idx++)
> +		values[idx] = -1;
>  
>  	/* Remove all blank characters ahead */
> -	while (isblank(*corelist))
> -		corelist++;
> +	while (isblank(*list))
> +		list++;
> +
> +	min = maxsize;
>  
> -	/* Get list of cores */
> -	min = RTE_MAX_LCORE;
>  	do {
> -		while (isblank(*corelist))
> -			corelist++;
> -		if (*corelist == '\0')
> +		while (isblank(*list))
> +			list++;
> +		if (*list == '\0')
>  			return -1;
>  		errno = 0;
> -		idx = strtol(corelist, &end, 10);
> +		idx = strtol(list, &end, 10);
>  		if (errno || end == NULL)
>  			return -1;
> -		if (idx < 0 || idx >= RTE_MAX_LCORE)
> +		if (idx < 0 || idx >= maxsize)
>  			return -1;
>  		while (isblank(*end))
>  			end++;
> @@ -605,18 +608,18 @@ eal_parse_corelist(const char *corelist, int *cores)
>  			min = idx;
>  		} else if ((*end == ',') || (*end == '\0')) {
>  			max = idx;
> -			if (min == RTE_MAX_LCORE)
> +			if (min == maxsize)
>  				min = idx;
>  			for (idx = min; idx <= max; idx++) {
> -				if (cores[idx] == -1) {
> -					cores[idx] = count;
> +				if (values[idx] == -1) {
> +					values[idx] = count;
>  					count++;
>  				}
>  			}
> -			min = RTE_MAX_LCORE;
> +			min = maxsize;
>  		} else
>  			return -1;
> -		corelist = end + 1;
> +		list = end + 1;
>  	} while (*end != '\0');
>  
>  	if (count == 0)
> @@ -624,6 +627,12 @@ eal_parse_corelist(const char *corelist, int *cores)
>  	return 0;
>  }
>  
> +static int
> +eal_parse_corelist(const char *corelist, int *cores)
> +{
> +	return eal_parse_optionlist(corelist, cores, RTE_MAX_LCORE);
> +}
> +
>  /* Changes the lcore id of the master thread */
>  static int
>  eal_parse_master_lcore(const char *arg)
> diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
> index 2f9ed29..567b754 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -71,6 +71,40 @@ enum rte_proc_type_t rte_eal_process_type(void);
>  int rte_eal_iopl_init(void);
>  
>  /**
> + * Parse the user input
> + *
> + * This function can be used to read and parse the user input
> + * from the command line. For example, when the user specifies
> + * corelist or port list this function will read the input
> + * and set the forwarding cores or ports
> + *
> + * @param[in] list
> + *   String containing the user input. User can specify
> + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + *   For example, if the user wants to use all the available
> + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + *   If the user wants to use only the ports 1,2 then the input
> + *   is 1,2.
> + *   valid characters are '-' and ','
> + *   invalid chars like '.' or '#' will result in
> + *   EAL: Error - exiting with code: 1
> + *     Cause: Invalid fwd port list
> + * @param[in] values
> + *   An array pointer, used by this function to set the
> + *   array contents to a positive value if they are listed
> + *   in the input
> + *   else sets it to -1
> + * @param[in] maxsize
> + *   This is the maximum value the list string can contain
> + * @return
> + *   -On success, returns 0.
> + *   -On failure, returns -1.
> + */
> +__rte_experimental
> +int
> +eal_parse_optionlist(const char *list, int *values, int maxsize);
> +
> +/**
>   * Initialize the Environment Abstraction Layer (EAL).
>   *
>   * This function is to be executed on the MASTER lcore only, as soon
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index e38d025..3d72df8 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -332,4 +332,5 @@ EXPERIMENTAL {
>  	# added in 19.11
>  	rte_log_get_stream;
>  	rte_mcfg_get_single_file_segments;
> +	eal_parse_optionlist;
>  };
> 

Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[David Marchand]

On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
> > In current version, there is a function which parses
> > the corelist based on user value. A new generic
> > function eal_parse_optionlist is added which will
> > parse corelist as well as similar user input so
> > that we can use it as a public API too.
> >
> > Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
>
> Hi David,
>
> Overall this patchset is to add '--portlist' command to testpmd and remove
> existing 64 port limitation.
>
> And in this patch re-uses the exiting parser function in eal and converts it to
> API, question is if eal is good place to have this API, what do you think about it?

Exporting string parsers from the EAL has little value.
Ok we avoid code duplication (and I can see other places in the tree
where it might be used), but in the end we will have to maintain this
API in the ABI when it enters the stable ABI.

I am for avoiding this.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[Ferruh Yigit]

On 1/29/2020 5:44 PM, David Marchand wrote:
> On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
>>> In current version, there is a function which parses
>>> the corelist based on user value. A new generic
>>> function eal_parse_optionlist is added which will
>>> parse corelist as well as similar user input so
>>> that we can use it as a public API too.
>>>
>>> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
>>
>> Hi David,
>>
>> Overall this patchset is to add '--portlist' command to testpmd and remove
>> existing 64 port limitation.
>>
>> And in this patch re-uses the exiting parser function in eal and converts it to
>> API, question is if eal is good place to have this API, what do you think about it?
> 
> Exporting string parsers from the EAL has little value.
> Ok we avoid code duplication (and I can see other places in the tree
> where it might be used), but in the end we will have to maintain this
> API in the ABI when it enters the stable ABI.
> 
> I am for avoiding this.
> 
>

The same function can be used in some sample applications too (which are using
port mask), so instead of duplicating it multiple times, it would be nice to
have this function somewhere that applications can use.

Does it makes sense to have a rte_util.c (under in eal or as a separate library)
to have this kind of application helper functions?

Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[Stephen Hemminger]

On Wed, 29 Jan 2020 18:07:05 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/29/2020 5:44 PM, David Marchand wrote:
> > On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:  
> >> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:  
> >>> In current version, there is a function which parses
> >>> the corelist based on user value. A new generic
> >>> function eal_parse_optionlist is added which will
> >>> parse corelist as well as similar user input so
> >>> that we can use it as a public API too.
> >>>
> >>> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>  
> >>
> >> Hi David,
> >>
> >> Overall this patchset is to add '--portlist' command to testpmd and remove
> >> existing 64 port limitation.
> >>
> >> And in this patch re-uses the exiting parser function in eal and converts it to
> >> API, question is if eal is good place to have this API, what do you think about it?  
> > 
> > Exporting string parsers from the EAL has little value.
> > Ok we avoid code duplication (and I can see other places in the tree
> > where it might be used), but in the end we will have to maintain this
> > API in the ABI when it enters the stable ABI.
> > 
> > I am for avoiding this.
> > 
> >  
> 
> The same function can be used in some sample applications too (which are using
> port mask), so instead of duplicating it multiple times, it would be nice to
> have this function somewhere that applications can use.
> 
> Does it makes sense to have a rte_util.c (under in eal or as a separate library)
> to have this kind of application helper functions?

It makes sense to have a rte library that handles arbitrary size bitvector
and has string handling functions. Kind of like what kernel has for
the cpuset parsing.  This could be used for cpus in EAL and port-masks
or other arrays in applications.

But just doing copy/paste of existing code without thinking about how
API should work is a bad idea.

Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[Govindharajan, Hariprasad]

Hi,
I am planning to move the existing parser function to the testpmd file instead of keeping it in the eal and will revert the eal back.

Also, I am planning to create an util file in eal with this parser and do a RFC.

@Stephen Hemminger, We already investigated the existing function and then decided to re-use it as seen in the patch. For creating an API, is there any other specific requirements 
that should be addressed? Please clarify us.

Thanks
G Hariprasad

-----Original Message-----
From: Stephen Hemminger <stephen@networkplumber.org> 
Sent: Wednesday, January 29, 2020 7:20 PM
To: Yigit, Ferruh <ferruh.yigit@intel.com>
Cc: David Marchand <david.marchand@redhat.com>; Thomas Monjalon <thomas@monjalon.net>; Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>; dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

On Wed, 29 Jan 2020 18:07:05 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/29/2020 5:44 PM, David Marchand wrote:
> > On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:  
> >> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:  
> >>> In current version, there is a function which parses the corelist 
> >>> based on user value. A new generic function eal_parse_optionlist 
> >>> is added which will parse corelist as well as similar user input 
> >>> so that we can use it as a public API too.
> >>>
> >>> Signed-off-by: Hariprasad Govindharajan 
> >>> <hariprasad.govindharajan@intel.com>
> >>
> >> Hi David,
> >>
> >> Overall this patchset is to add '--portlist' command to testpmd and 
> >> remove existing 64 port limitation.
> >>
> >> And in this patch re-uses the exiting parser function in eal and 
> >> converts it to API, question is if eal is good place to have this API, what do you think about it?
> > 
> > Exporting string parsers from the EAL has little value.
> > Ok we avoid code duplication (and I can see other places in the tree 
> > where it might be used), but in the end we will have to maintain 
> > this API in the ABI when it enters the stable ABI.
> > 
> > I am for avoiding this.
> > 
> >  
> 
> The same function can be used in some sample applications too (which 
> are using port mask), so instead of duplicating it multiple times, it 
> would be nice to have this function somewhere that applications can use.
> 
> Does it makes sense to have a rte_util.c (under in eal or as a 
> separate library) to have this kind of application helper functions?

It makes sense to have a rte library that handles arbitrary size bitvector and has string handling functions. Kind of like what kernel has for the cpuset parsing.  This could be used for cpus in EAL and port-masks or other arrays in applications.

But just doing copy/paste of existing code without thinking about how API should work is a bad idea.

Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

[Govindharajan, Hariprasad]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, January 29, 2020 7:20 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: David Marchand <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>; Govindharajan, Hariprasad
> <hariprasad.govindharajan@intel.com>; dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse
> user input
> 
> On Wed, 29 Jan 2020 18:07:05 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 1/29/2020 5:44 PM, David Marchand wrote:
> > > On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> > >> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
> > >>> In current version, there is a function which parses the corelist
> > >>> based on user value. A new generic function eal_parse_optionlist
> > >>> is added which will parse corelist as well as similar user input
> > >>> so that we can use it as a public API too.
> > >>>
> > >>> Signed-off-by: Hariprasad Govindharajan
> > >>> <hariprasad.govindharajan@intel.com>
> > >>
> > >> Hi David,
> > >>
> > >> Overall this patchset is to add '--portlist' command to testpmd and
> > >> remove existing 64 port limitation.
> > >>
> > >> And in this patch re-uses the exiting parser function in eal and
> > >> converts it to API, question is if eal is good place to have this API, what
> do you think about it?
> > >
> > > Exporting string parsers from the EAL has little value.
> > > Ok we avoid code duplication (and I can see other places in the tree
> > > where it might be used), but in the end we will have to maintain
> > > this API in the ABI when it enters the stable ABI.
> > >
> > > I am for avoiding this.
> > >
> > >
> >
> > The same function can be used in some sample applications too (which
> > are using port mask), so instead of duplicating it multiple times, it
> > would be nice to have this function somewhere that applications can use.
> >
> > Does it makes sense to have a rte_util.c (under in eal or as a
> > separate library) to have this kind of application helper functions?
> 
> It makes sense to have a rte library that handles arbitrary size bitvector and
> has string handling functions. Kind of like what kernel has for the cpuset
> parsing.  This could be used for cpus in EAL and port-masks or other arrays in
> applications.
> 
> But just doing copy/paste of existing code without thinking about how API
> should work is a bad idea.

[Govindharajan, Hariprasad] 
Hi,

PLEASE IGNORE MY PREVIOUS EMAIL.

I am planning to move the existing parser function to the testpmd file instead of keeping it in the eal and will revert the eal back.

Also, I am planning to create an util file in eal with this parser and do a RFC.

@Stephen Hemminger, We already investigated the existing function and then decided to re-use it as seen in the patch. For creating an API, is there any other specific requirements that should be addressed? Please clarify us.

Thanks
G Hariprasad