Hi Andrew,

-----Original Message-----
From: Andrew Rybchenko <redacted>
Sent: Tuesday, January 19, 2021 3:46 PM
To: Xueming(Steven) Li <redacted>; NBU-Contact-Thomas
Monjalon [off-list ref]; Ferruh Yigit [off-list ref];
Olivier Matz [off-list ref]
Cc: dev@dpdk.org; Slava Ovsiienko <redacted>; Asaf Penso
[off-list ref]
Subject: Re: [PATCH v4 2/9] ethdev: support representor port list

On 1/18/21 2:16 PM, Xueming Li wrote:

quoted

To support extended representor syntax, this patch extends the
representor list parsing to support for representor port range in
devargs, examples:
   representor=[1,2,3]         - single list
   representor=[1,3-5,7,9-11]  - list with singles and ranges

Signed-off-by: Xueming Li <redacted>
Acked-by: Viacheslav Ovsiienko <redacted>

See below

quoted

---
 lib/librte_ethdev/ethdev_private.c | 105 ++++++++++++++---------------
 lib/librte_ethdev/ethdev_private.h |   3 -
 lib/librte_ethdev/rte_class_eth.c  |   4 +-
 lib/librte_ethdev/rte_ethdev.c     |   5 +-
 4 files changed, 54 insertions(+), 63 deletions(-)

diff --git a/lib/librte_ethdev/ethdev_private.c

b/lib/librte_ethdev/ethdev_private.c
index c1a411dba4..12bcc7e98d 100644

--- a/lib/librte_ethdev/ethdev_private.c
+++ b/lib/librte_ethdev/ethdev_private.c

@@ -38,77 +38,71 @@ eth_find_device(const struct rte_eth_dev *start,

rte_eth_cmp_t cmp,

quoted

 	return NULL;
 }

-int
-rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
-	void *data)
+static int
+rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list,
+		       const uint16_t max_list, uint16_t val)
 {
-	char *str_start;
-	int state;
-	int result;
-
-	if (*str != '[')
-		/* Single element, not a list */
-		return callback(str, data);
-
-	/* Sanity check, then strip the brackets */
-	str_start = &str[strlen(str) - 1];
-	if (*str_start != ']') {
-		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
-		return -EINVAL;
-	}
-	str++;
-	*str_start = '\0';
+	uint16_t i;

-	/* Process list elements */
-	state = 0;
-	while (1) {
-		if (state == 0) {
-			if (*str == '\0')
-				break;
-			if (*str != ',') {
-				str_start = str;
-				state = 1;
-			}
-		} else if (state == 1) {
-			if (*str == ',' || *str == '\0') {
-				if (str > str_start) {
-					/* Non-empty string fragment */
-					*str = '\0';
-					result = callback(str_start, data);
-					if (result < 0)
-						return result;
-				}
-				state = 0;
-			}
-		}
-		str++;
+	if (*len_list >= max_list)
+		return -1;

If current length is equal to max, but added value is already is in the list, it
should not be an error. So, these two lines should be moved after below for
loop.

quoted

+	for (i = 0; i < *len_list; i++) {
+		if (list[i] == val)
+			return 0;
 	}
+	list[(*len_list)++] = val;
 	return 0;
 }

-static int
+static char *
 rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
 	const uint16_t max_list)
 {
 	uint16_t lo, hi, val;
 	int result;
+	char *pos = str;

 	result = sscanf(str, "%hu-%hu", &lo, &hi);
 	if (result == 1) {
-		if (*len_list >= max_list)
-			return -ENOMEM;
-		list[(*len_list)++] = lo;
+		if (rte_eth_devargs_enlist(list, len_list, max_list, lo) != 0)
+			return NULL;
 	} else if (result == 2) {
-		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi >

RTE_MAX_ETHPORTS)

Strictly speaking removal of comparision vs RTE_MAX_ETHPORTS is a separate
logical change with a separate motivation.

To make this function comment for controller/pf/vf/sf parsing, the max value is
passed in as parameter and checked in rte_eth_devargs_enlist().
Caller code in rte_eth_devargs_process_list() decide the max value.

quoted

-			return -EINVAL;
+		if (lo >= hi)

I'd remove '=' here. It should not be a problem and handed perfectly by below
code. I see no point to deny 3-3 range which is an equivalent for just 3. It
could be convenient in some cases.

quoted

+			return NULL;
 		for (val = lo; val <= hi; val++) {
-			if (*len_list >= max_list)
-				return -ENOMEM;
-			list[(*len_list)++] = val;
+			if (rte_eth_devargs_enlist(list, len_list, max_list,
+						   val) != 0)
+				return NULL;
 		}
 	} else
-		return -EINVAL;
-	return 0;
+		return NULL;
+	while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-'))

*post != '\0' is a bit better looking at subsequent comparisons. Yes, it is just
style. Up to you.

quoted

+		pos++;

It looks too fragile. May I suggest to use %n in above scanf to be able to skip
only parsed characters.

quoted

+	return pos;
+}
+
+static char *
+rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
+	const uint16_t max_list)
+{
+	char *pos = str;
+
+	if (*pos == '[')
+		pos++;
+	while (1) {
+		pos = rte_eth_devargs_process_range(pos, list, len_list,
+						    max_list);
+		if (pos == NULL)
+			return NULL;
+		if (*pos != ',') /* end of list */
+			break;
+		pos++;
+	}
+	if (*str == '[' && *pos != ']')
+		return NULL;
+	if (*pos == ']')
+		pos++;
+	return pos;
 }

 /*

@@ -121,6 +115,9 @@ rte_eth_devargs_parse_representor_ports(char *str,

void *data)

quoted

 	struct rte_eth_devargs *eth_da = data;

 	eth_da->type = RTE_ETH_REPRESENTOR_VF;
-	return rte_eth_devargs_process_range(str, eth_da-
representor_ports,
+	str = rte_eth_devargs_process_list(str, eth_da->representor_ports,
 		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);

Not directly related to the patch, but I dislike RTE_MAX_ETHPORTS above.
RTE_DIM(eth_da->representor_ports) would be more readable.

The array dim could be different than RTE_MAX_ETHPORTS, although they are
same today 😊

quoted

+	if (str == NULL)
+		RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
+	return str == NULL ? -1 : 0;
 }

[snip]

Other comments looks great, will update, thanks!