[v9,05/10] kvargs: update parser to support multiple lists

Message ID 1615468416-10115-6-git-send-email-xuemingl@nvidia.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: support SubFunction representor |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Xueming Li March 11, 2021, 1:13 p.m. UTC
  This patch updates kvargs parser to support value of multiple lists or
ranges:
  k1=v[1,2]v[3-5]

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test/test_kvargs.c         |  46 +++++++++++++--
 lib/librte_kvargs/rte_kvargs.c | 101 +++++++++++++++++++++++----------
 2 files changed, 112 insertions(+), 35 deletions(-)
  

Comments

Ray Kinsella April 12, 2021, 4:42 p.m. UTC | #1
On 11/03/2021 13:13, Xueming Li wrote:
> This patch updates kvargs parser to support value of multiple lists or
> ranges:
>   k1=v[1,2]v[3-5]
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  app/test/test_kvargs.c         |  46 +++++++++++++--
>  lib/librte_kvargs/rte_kvargs.c | 101 +++++++++++++++++++++++----------
>  2 files changed, 112 insertions(+), 35 deletions(-)
> 

Hi folks,

This is essentially an FYI.
This change introduced a weak functional ABI regression into DPDK.

When I test librte_kvargs using the v20.11 unit test binary I get the following.

EAL: Detected 80 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Detected shared linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'PA'
EAL: 4090 hugepages of size 2097152 reserved, but no mounted hugetlbfs found for that size
APP: HPET is not enabled, using TSC as default timer
RTE>>kvargs_autotest
== test valid case ==
== test invalid case ==
rte_kvargs_parse() returned 0 (but should not)
while processing <foo=1,foo=> using valid_keys=<foo,check>
Test Failed
RTE>>

The reason this is failing in v20.11 and passing at the HEAD, 
is that "no value" test case (foo=1,foo=) were removed from test_invalid_kvargs, and was added to test_valid_kvargs.

So tokens that were definitely invalid in v20.11, are now is valid in v21.11.
My 2c is that as long as the valid values in v20.11 are still OK, there is no regression.

Thanks,

Ray K
  

Patch

diff --git a/app/test/test_kvargs.c b/app/test/test_kvargs.c
index 2a2dae43a0..a91ea8dc47 100644
--- a/app/test/test_kvargs.c
+++ b/app/test/test_kvargs.c
@@ -35,6 +35,25 @@  static int check_handler(const char *key, const char *value,
 	return 0;
 }
 
+/* test parsing. */
+static int test_kvargs_parsing(const char *args, unsigned int n)
+{
+	struct rte_kvargs *kvlist;
+
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_kvargs_parse() error: %s\n", args);
+		return -1;
+	}
+	if (kvlist->count != n) {
+		printf("invalid count value %d: %s\n", kvlist->count, args);
+		rte_kvargs_free(kvlist);
+		return -1;
+	}
+	rte_kvargs_free(kvlist);
+	return 0;
+}
+
 /* test a valid case */
 static int test_valid_kvargs(void)
 {
@@ -42,6 +61,19 @@  static int test_valid_kvargs(void)
 	const char *args;
 	const char *valid_keys_list[] = { "foo", "check", NULL };
 	const char **valid_keys;
+	static const struct {
+		unsigned int expected;
+		const char *input;
+	} valid_inputs[] = {
+		{ 2, "foo=1,foo=" },
+		{ 2, "foo=1,foo=" },
+		{ 2, "foo=1,foo" },
+		{ 2, "foo=1,=2" },
+		{ 1, "foo=[1,2" },
+		{ 1, ",=" },
+		{ 1, "foo=[" },
+	};
+	unsigned int i;
 
 	/* empty args is valid */
 	args = "";
@@ -191,6 +223,14 @@  static int test_valid_kvargs(void)
 	}
 	rte_kvargs_free(kvlist);
 
+	valid_keys = NULL;
+
+	for (i = 0; i < RTE_DIM(valid_inputs); ++i) {
+		args = valid_inputs[i].input;
+		if (test_kvargs_parsing(args, valid_inputs[i].expected))
+			goto fail;
+	}
+
 	return 0;
 
  fail:
@@ -212,12 +252,6 @@  static int test_invalid_kvargs(void)
 	/* list of argument that should fail */
 	const char *args_list[] = {
 		"wrong-key=x",     /* key not in valid_keys_list */
-		"foo=1,foo=",      /* empty value */
-		"foo=1,foo",       /* no value */
-		"foo=1,=2",        /* no key */
-		"foo=[1,2",        /* no closing bracket in value */
-		",=",              /* also test with a smiley */
-		"foo=[",           /* no value in list and no closing bracket */
 		NULL };
 	const char **args;
 	const char *valid_keys_list[] = { "foo", "check", NULL };
diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index 285081c86c..ffae8914cf 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -5,6 +5,7 @@ 
 
 #include <string.h>
 #include <stdlib.h>
+#include <stdbool.h>
 
 #include <rte_string_fns.h>
 
@@ -13,15 +14,19 @@ 
 /*
  * Receive a string with a list of arguments following the pattern
  * key=value,key=value,... and insert them into the list.
- * strtok() is used so the params string will be copied to be modified.
+ * Params string will be copied to be modified.
+ * list "[]" and list element splitter ",", "-" is treated as value.
+ * Supported examples:
+ *   k1=v1,k2=v2
+ *   k1
+ *   k1=x[0-1]y[1,3-5,9]z
  */
 static int
 rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 {
-	unsigned i;
-	char *str;
-	char *ctx1 = NULL;
-	char *ctx2 = NULL;
+	char *str, *start;
+	bool in_list = false, end_key = false, end_value = false;
+	bool save = false, end_pair = false;
 
 	/* Copy the const char *params to a modifiable string
 	 * to pass to rte_strsplit
@@ -32,36 +37,74 @@  rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 
 	/* browse each key/value pair and add it in kvlist */
 	str = kvlist->str;
-	while ((str = strtok_r(str, RTE_KVARGS_PAIRS_DELIM, &ctx1)) != NULL) {
+	start = str; /* start of current key or value */
+	while (1) {
+		switch (*str) {
+		case '=': /* End of key. */
+			end_key = true;
+			save = true;
+			break;
+		case ',':
+			/* End of value, skip comma in middle of range */
+			if (!in_list) {
+				if (end_key)
+					end_value = true;
+				else
+					end_key = true;
+				save = true;
+				end_pair = true;
+			}
+			break;
+		case '[': /* Start of list. */
+			in_list = true;
+			break;
+		case ']': /* End of list.  */
+			if (in_list)
+				in_list = false;
+			break;
+		case '\0': /* End of string */
+			if (end_key)
+				end_value = true;
+			else
+				end_key = true;
+			save = true;
+			end_pair = true;
+			break;
+		default:
+			break;
+		}
 
-		i = kvlist->count;
-		if (i >= RTE_KVARGS_MAX)
-			return -1;
+		if (!save) {
+			/* Continue if not end of key or value. */
+			str++;
+			continue;
+		}
 
-		kvlist->pairs[i].key = strtok_r(str, RTE_KVARGS_KV_DELIM, &ctx2);
-		kvlist->pairs[i].value = strtok_r(NULL, RTE_KVARGS_KV_DELIM, &ctx2);
-		if (kvlist->pairs[i].key == NULL ||
-		    kvlist->pairs[i].value == NULL)
+		if (kvlist->count >= RTE_KVARGS_MAX)
 			return -1;
 
-		/* Detect list [a,b] to skip comma delimiter in list. */
-		str = kvlist->pairs[i].value;
-		if (str[0] == '[') {
-			/* Find the end of the list. */
-			while (str[strlen(str) - 1] != ']') {
-				/* Restore the comma erased by strtok_r(). */
-				if (ctx1 == NULL || ctx1[0] == '\0')
-					return -1; /* no closing bracket */
-				str[strlen(str)] = ',';
-				/* Parse until next comma. */
-				str = strtok_r(NULL, RTE_KVARGS_PAIRS_DELIM, &ctx1);
-				if (str == NULL)
-					return -1; /* no closing bracket */
-			}
+		if (end_value)
+			/* Value parsed */
+			kvlist->pairs[kvlist->count].value = start;
+		else if (end_key)
+			/* Key parsed. */
+			kvlist->pairs[kvlist->count].key = start;
+
+		if (end_pair) {
+			if (end_value || str != start)
+				/* Ignore empty pair. */
+				kvlist->count++;
+			end_key = false;
+			end_value = false;
+			end_pair = false;
 		}
 
-		kvlist->count++;
-		str = NULL;
+		if (*str == '\0') /* End of string. */
+			break;
+		*str = '\0';
+		str++;
+		start = str;
+		save = false;
 	}
 
 	return 0;