DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments
@ 2021-10-05 12:30 Xueming Li
  2021-10-05 12:30 ` [dpdk-dev] [PATCH 2/3] devargs: make bus key parsing optional Xueming Li
                   ` (5 more replies)
  0 siblings, 6 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-05 12:30 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand

Slash is used to split global device arguments.

To support path value which contains slash, this patch parses devargs by
locating both slash and layer name key:
  bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
"/class=" and "/driver" are valid start of a layer.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 lib/eal/common/eal_common_devargs.c | 117 ++++++++++------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 7ab9e71b2a3..e3786c6c02a 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -29,18 +29,6 @@ TAILQ_HEAD(rte_devargs_list, rte_devargs);
 static struct rte_devargs_list devargs_list =
 	TAILQ_HEAD_INITIALIZER(devargs_list);
 
-static size_t
-devargs_layer_count(const char *s)
-{
-	size_t i = s ? 1 : 0;
-
-	while (s != NULL && s[0] != '\0') {
-		i += s[0] == '/';
-		s++;
-	}
-	return i;
-}
-
 /* Resolve devargs name from bus arguments. */
 static int
 devargs_bus_parse_default(struct rte_devargs *devargs,
@@ -77,23 +65,13 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
 	};
 	struct rte_kvargs_pair *kv = NULL;
-	struct rte_class *cls = NULL;
-	struct rte_bus *bus = NULL;
-	const char *s = devstr;
-	size_t nblayer;
-	size_t i = 0;
+	struct rte_kvargs *bus_kvlist = NULL;
+	char *s;
+	size_t nblayer = 0;
+	size_t i;
 	int ret = 0;
 	bool allocated_data = false;
 
-	/* Split each sub-lists. */
-	nblayer = devargs_layer_count(devstr);
-	if (nblayer > RTE_DIM(layers)) {
-		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
-			nblayer);
-		ret = -E2BIG;
-		goto get_out;
-	}
-
 	/* If the devargs points the devstr
 	 * as source data, then it should not allocate
 	 * anything and keep referring only to it.
@@ -106,33 +84,41 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 			goto get_out;
 		}
 		allocated_data = true;
-		s = devargs->data;
 	}
+	s = devargs->data;
 
 	while (s != NULL) {
-		if (i >= RTE_DIM(layers)) {
-			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
-			ret = -EINVAL;
+		if (nblayer > RTE_DIM(layers)) {
+			ret = -E2BIG;
 			goto get_out;
 		}
-		/*
-		 * The last layer is free-form.
-		 * The "driver" key is not required (but accepted).
-		 */
-		if (strncmp(layers[i].key, s, strlen(layers[i].key)) &&
-				i != RTE_DIM(layers) - 1)
-			goto next_layer;
-		layers[i].str = s;
-		layers[i].kvlist = rte_kvargs_parse_delim(s, NULL, "/");
-		if (layers[i].kvlist == NULL) {
+		layers[nblayer].str = s;
+
+		/* Locate next layer starts with valid layer key. */
+		while (s != NULL) {
+			s = strchr(s, '/');
+			if (s == NULL)
+				break;
+			for (i = 0; i < RTE_DIM(layers); i++) {
+				if (strncmp(s + 1, layers[i].key,
+					    strlen(layers[i].key)) == 0) {
+					*s = '\0';
+					break;
+				}
+			}
+			s++;
+			if (i < RTE_DIM(layers))
+				break;
+		}
+
+		layers[nblayer].kvlist = rte_kvargs_parse
+				(layers[nblayer].str, NULL);
+		if (layers[nblayer].kvlist == NULL) {
 			ret = -EINVAL;
 			goto get_out;
 		}
-		s = strchr(s, '/');
-		if (s != NULL)
-			s++;
-next_layer:
-		i++;
+
+		nblayer++;
 	}
 
 	/* Parse each sub-list. */
@@ -143,52 +129,35 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (kv->key == NULL)
 			continue;
 		if (strcmp(kv->key, RTE_DEVARGS_KEY_BUS) == 0) {
-			bus = rte_bus_find_by_name(kv->value);
-			if (bus == NULL) {
+			bus_kvlist = layers[i].kvlist;
+			devargs->bus_str = layers[i].str;
+			devargs->bus = rte_bus_find_by_name(kv->value);
+			if (devargs->bus == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_CLASS) == 0) {
-			cls = rte_class_find_by_name(kv->value);
-			if (cls == NULL) {
+			devargs->cls_str = layers[i].str;
+			devargs->cls = rte_class_find_by_name(kv->value);
+			if (devargs->cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_DRIVER) == 0) {
-			/* Ignore */
+			devargs->drv_str = layers[i].str;
 			continue;
 		}
 	}
 
-	/* Fill devargs fields. */
-	devargs->bus_str = layers[0].str;
-	devargs->cls_str = layers[1].str;
-	devargs->drv_str = layers[2].str;
-	devargs->bus = bus;
-	devargs->cls = cls;
-
-	/* If we own the data, clean up a bit
-	 * the several layers string, to ease
-	 * their parsing afterward.
-	 */
-	if (devargs->data != devstr) {
-		char *s = devargs->data;
-
-		while ((s = strchr(s, '/'))) {
-			*s = '\0';
-			s++;
-		}
-	}
-
 	/* Resolve devargs name. */
-	if (bus != NULL && bus->devargs_parse != NULL)
-		ret = bus->devargs_parse(devargs);
-	else if (layers[0].kvlist != NULL)
-		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
+		ret = devargs->bus->devargs_parse(devargs);
+	else if (bus_kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, bus_kvlist);
 
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
-- 
2.33.0


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

* [dpdk-dev] [PATCH 2/3] devargs: make bus key parsing optional
  2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-05 12:30 ` Xueming Li
  2021-10-05 12:30 ` [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases Xueming Li
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-05 12:30 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand, stable, Gaetan Rivet

Global devargs syntax is used as device iteration filter like
"class=vdpa", a devargs without bus args is valid from parsing
perspective.

This patch makes bus args optional.

Fixes: d2a66ad79480 ("bus: add device arguments name parsing")
Cc: stable@dpdk.org

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 drivers/bus/pci/pci_params.c        | 8 +++-----
 lib/eal/common/eal_common_devargs.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 691b5ea0180..f6fa3a5d6ce 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -85,11 +85,10 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 	struct rte_kvargs *kvargs;
 	const char *addr_str;
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = 0;
 
-	if (da == NULL)
+	if (da == NULL || da->bus_str == NULL)
 		return 0;
-	RTE_ASSERT(da->bus_str != NULL);
 
 	kvargs = rte_kvargs_parse(da->bus_str, NULL);
 	if (kvargs == NULL) {
@@ -101,9 +100,8 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 
 	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
 	if (addr_str == NULL) {
-		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+		RTE_LOG(DEBUG, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
 			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
-		ret = -ENODEV;
 		goto out;
 	}
 
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index e3786c6c02a..616cf77f229 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -39,7 +39,7 @@ devargs_bus_parse_default(struct rte_devargs *devargs,
 	/* Parse devargs name from bus key-value list. */
 	name = rte_kvargs_get(bus_args, "name");
 	if (name == NULL) {
-		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+		RTE_LOG(DEBUG, EAL, "devargs name not found: %s\n",
 			devargs->data);
 		return 0;
 	}
-- 
2.33.0


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

* [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases
  2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-05 12:30 ` [dpdk-dev] [PATCH 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-05 12:30 ` Xueming Li
  2021-10-05 14:01   ` David Marchand
  2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-05 12:30 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand

Initial version to test Global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/meson.build    |   1 +
 app/test/test_devargs.c | 147 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+)
 create mode 100644 app/test/test_devargs.c

diff --git a/app/test/meson.build b/app/test/meson.build
index f144d8b8ed6..c688ba2b70a 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -42,6 +42,7 @@ test_sources = files(
         'test_cryptodev_security_pdcp.c',
         'test_cycles.c',
         'test_debug.c',
+        'test_devargs.c',
         'test_distributor.c',
         'test_distributor_perf.c',
         'test_eal_flags.c',
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..8a173368347
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,147 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct {
+		const char *devargs;
+		int bus_kv;
+		int class_kv;
+		int driver_kv;
+	} list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci", 1, 0, 0 },
+		{ "class=eth", 0, 1, 0 },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0", 2, 1, 2 },
+		{ "bus=vdev,name=/dev/file/name/class=eth", 2, 1, 0 },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0 },
+		{ "pci:1:2.3,k0=v0", 0, 0, 1 },
+		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+		  0, 0, 3 },
+	};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if (list[i].bus_kv > 0 && da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].class_kv > 0 && da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		goto cleanup;
+fail:
+		fail = -1;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases
  2021-10-05 12:30 ` [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-05 14:01   ` David Marchand
  2021-10-05 15:56     ` Xueming(Steven) Li
  0 siblings, 1 reply; 43+ messages in thread
From: David Marchand @ 2021-10-05 14:01 UTC (permalink / raw)
  To: Xueming Li; +Cc: dev, Thomas Monjalon

On Tue, Oct 5, 2021 at 2:31 PM Xueming Li <xuemingl@nvidia.com> wrote:
> diff --git a/app/test/meson.build b/app/test/meson.build
> index f144d8b8ed6..c688ba2b70a 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -42,6 +42,7 @@ test_sources = files(
>          'test_cryptodev_security_pdcp.c',
>          'test_cycles.c',
>          'test_debug.c',
> +        'test_devargs.c',
>          'test_distributor.c',
>          'test_distributor_perf.c',
>          'test_eal_flags.c',

You likely want to add devargs_autotest to fast_tests, else "meson
test" (and the CI) won't execute this new test.
As for tests referenced in autotest_data.py, I am not sure we really
need this, but please update too for consistency.


-- 
David Marchand


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

* [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax
  2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-05 12:30 ` [dpdk-dev] [PATCH 2/3] devargs: make bus key parsing optional Xueming Li
  2021-10-05 12:30 ` [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-05 15:54 ` Xueming Li
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments Xueming Li
                     ` (3 more replies)
  2021-10-20  8:21 ` [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax Xueming Li
                   ` (2 subsequent siblings)
  5 siblings, 4 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-05 15:54 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand

- Support path in global syntax.
- Fix bus name resolving
- Add devargs test cases

v1: add test cases to test suite

Xueming Li (3):
  devargs: support path value for global device arguments
  devargs: make bus key parsing optional
  test/devargs: add devargs test cases

 app/test/autotest_data.py           |   6 ++
 app/test/meson.build                |   2 +
 app/test/test_devargs.c             | 147 ++++++++++++++++++++++++++++
 drivers/bus/pci/pci_params.c        |   8 +-
 lib/eal/common/eal_common_devargs.c | 119 +++++++++-------------
 5 files changed, 202 insertions(+), 80 deletions(-)
 create mode 100644 app/test/test_devargs.c

-- 
2.33.0


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

* [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments
  2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-05 15:54   ` Xueming Li
  2021-10-19 14:57     ` Gaëtan Rivet
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 2/3] devargs: make bus key parsing optional Xueming Li
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-05 15:54 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand

Slash is used to split global device arguments.

To support path value which contains slash, this patch parses devargs by
locating both slash and layer name key:
  bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
"/class=" and "/driver" are valid start of a layer.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 lib/eal/common/eal_common_devargs.c | 117 ++++++++++------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 7ab9e71b2a3..e3786c6c02a 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -29,18 +29,6 @@ TAILQ_HEAD(rte_devargs_list, rte_devargs);
 static struct rte_devargs_list devargs_list =
 	TAILQ_HEAD_INITIALIZER(devargs_list);
 
-static size_t
-devargs_layer_count(const char *s)
-{
-	size_t i = s ? 1 : 0;
-
-	while (s != NULL && s[0] != '\0') {
-		i += s[0] == '/';
-		s++;
-	}
-	return i;
-}
-
 /* Resolve devargs name from bus arguments. */
 static int
 devargs_bus_parse_default(struct rte_devargs *devargs,
@@ -77,23 +65,13 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
 	};
 	struct rte_kvargs_pair *kv = NULL;
-	struct rte_class *cls = NULL;
-	struct rte_bus *bus = NULL;
-	const char *s = devstr;
-	size_t nblayer;
-	size_t i = 0;
+	struct rte_kvargs *bus_kvlist = NULL;
+	char *s;
+	size_t nblayer = 0;
+	size_t i;
 	int ret = 0;
 	bool allocated_data = false;
 
-	/* Split each sub-lists. */
-	nblayer = devargs_layer_count(devstr);
-	if (nblayer > RTE_DIM(layers)) {
-		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
-			nblayer);
-		ret = -E2BIG;
-		goto get_out;
-	}
-
 	/* If the devargs points the devstr
 	 * as source data, then it should not allocate
 	 * anything and keep referring only to it.
@@ -106,33 +84,41 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 			goto get_out;
 		}
 		allocated_data = true;
-		s = devargs->data;
 	}
+	s = devargs->data;
 
 	while (s != NULL) {
-		if (i >= RTE_DIM(layers)) {
-			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
-			ret = -EINVAL;
+		if (nblayer > RTE_DIM(layers)) {
+			ret = -E2BIG;
 			goto get_out;
 		}
-		/*
-		 * The last layer is free-form.
-		 * The "driver" key is not required (but accepted).
-		 */
-		if (strncmp(layers[i].key, s, strlen(layers[i].key)) &&
-				i != RTE_DIM(layers) - 1)
-			goto next_layer;
-		layers[i].str = s;
-		layers[i].kvlist = rte_kvargs_parse_delim(s, NULL, "/");
-		if (layers[i].kvlist == NULL) {
+		layers[nblayer].str = s;
+
+		/* Locate next layer starts with valid layer key. */
+		while (s != NULL) {
+			s = strchr(s, '/');
+			if (s == NULL)
+				break;
+			for (i = 0; i < RTE_DIM(layers); i++) {
+				if (strncmp(s + 1, layers[i].key,
+					    strlen(layers[i].key)) == 0) {
+					*s = '\0';
+					break;
+				}
+			}
+			s++;
+			if (i < RTE_DIM(layers))
+				break;
+		}
+
+		layers[nblayer].kvlist = rte_kvargs_parse
+				(layers[nblayer].str, NULL);
+		if (layers[nblayer].kvlist == NULL) {
 			ret = -EINVAL;
 			goto get_out;
 		}
-		s = strchr(s, '/');
-		if (s != NULL)
-			s++;
-next_layer:
-		i++;
+
+		nblayer++;
 	}
 
 	/* Parse each sub-list. */
@@ -143,52 +129,35 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (kv->key == NULL)
 			continue;
 		if (strcmp(kv->key, RTE_DEVARGS_KEY_BUS) == 0) {
-			bus = rte_bus_find_by_name(kv->value);
-			if (bus == NULL) {
+			bus_kvlist = layers[i].kvlist;
+			devargs->bus_str = layers[i].str;
+			devargs->bus = rte_bus_find_by_name(kv->value);
+			if (devargs->bus == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_CLASS) == 0) {
-			cls = rte_class_find_by_name(kv->value);
-			if (cls == NULL) {
+			devargs->cls_str = layers[i].str;
+			devargs->cls = rte_class_find_by_name(kv->value);
+			if (devargs->cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_DRIVER) == 0) {
-			/* Ignore */
+			devargs->drv_str = layers[i].str;
 			continue;
 		}
 	}
 
-	/* Fill devargs fields. */
-	devargs->bus_str = layers[0].str;
-	devargs->cls_str = layers[1].str;
-	devargs->drv_str = layers[2].str;
-	devargs->bus = bus;
-	devargs->cls = cls;
-
-	/* If we own the data, clean up a bit
-	 * the several layers string, to ease
-	 * their parsing afterward.
-	 */
-	if (devargs->data != devstr) {
-		char *s = devargs->data;
-
-		while ((s = strchr(s, '/'))) {
-			*s = '\0';
-			s++;
-		}
-	}
-
 	/* Resolve devargs name. */
-	if (bus != NULL && bus->devargs_parse != NULL)
-		ret = bus->devargs_parse(devargs);
-	else if (layers[0].kvlist != NULL)
-		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
+		ret = devargs->bus->devargs_parse(devargs);
+	else if (bus_kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, bus_kvlist);
 
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
-- 
2.33.0


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

* [dpdk-dev] [PATCH v1 2/3] devargs: make bus key parsing optional
  2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-05 15:54   ` Xueming Li
  2021-10-19 15:19     ` Gaëtan Rivet
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases Xueming Li
  2021-10-20  7:31   ` [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax Xueming Li
  3 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-05 15:54 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand, stable, Gaetan Rivet

Global devargs syntax is used as device iteration filter like
"class=vdpa", a devargs without bus args is valid from parsing
perspective.

This patch makes bus args optional.

Fixes: d2a66ad79480 ("bus: add device arguments name parsing")
Cc: stable@dpdk.org

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 drivers/bus/pci/pci_params.c        | 8 +++-----
 lib/eal/common/eal_common_devargs.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 691b5ea0180..f6fa3a5d6ce 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -85,11 +85,10 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 	struct rte_kvargs *kvargs;
 	const char *addr_str;
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = 0;
 
-	if (da == NULL)
+	if (da == NULL || da->bus_str == NULL)
 		return 0;
-	RTE_ASSERT(da->bus_str != NULL);
 
 	kvargs = rte_kvargs_parse(da->bus_str, NULL);
 	if (kvargs == NULL) {
@@ -101,9 +100,8 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 
 	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
 	if (addr_str == NULL) {
-		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+		RTE_LOG(DEBUG, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
 			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
-		ret = -ENODEV;
 		goto out;
 	}
 
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index e3786c6c02a..616cf77f229 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -39,7 +39,7 @@ devargs_bus_parse_default(struct rte_devargs *devargs,
 	/* Parse devargs name from bus key-value list. */
 	name = rte_kvargs_get(bus_args, "name");
 	if (name == NULL) {
-		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+		RTE_LOG(DEBUG, EAL, "devargs name not found: %s\n",
 			devargs->data);
 		return 0;
 	}
-- 
2.33.0


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

* [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases
  2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-05 15:54   ` Xueming Li
  2021-10-19 15:07     ` Gaëtan Rivet
  2021-10-20  7:31   ` [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax Xueming Li
  3 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-05 15:54 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Thomas Monjalon, David Marchand

Initial version to test Global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/autotest_data.py |   6 ++
 app/test/meson.build      |   2 +
 app/test/test_devargs.c   | 147 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 app/test/test_devargs.c

diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 302d6374c16..3b841e71918 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -785,6 +785,12 @@
         "Func":    default_autotest,
         "Report":  None,
     },
+    {
+        "Name":    "Devargs autotest",
+        "Command": "devargs_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
     #
     # Please always make sure that ring_perf is the last test!
     #
diff --git a/app/test/meson.build b/app/test/meson.build
index f144d8b8ed6..de8540f6119 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -42,6 +42,7 @@ test_sources = files(
         'test_cryptodev_security_pdcp.c',
         'test_cycles.c',
         'test_debug.c',
+        'test_devargs.c',
         'test_distributor.c',
         'test_distributor_perf.c',
         'test_eal_flags.c',
@@ -201,6 +202,7 @@ fast_tests = [
         ['common_autotest', true],
         ['cpuflags_autotest', true],
         ['debug_autotest', true],
+        ['devargs_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
         ['eal_flags_n_opt_autotest', false],
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..8a173368347
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,147 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct {
+		const char *devargs;
+		int bus_kv;
+		int class_kv;
+		int driver_kv;
+	} list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci", 1, 0, 0 },
+		{ "class=eth", 0, 1, 0 },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0", 2, 1, 2 },
+		{ "bus=vdev,name=/dev/file/name/class=eth", 2, 1, 0 },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0 },
+		{ "pci:1:2.3,k0=v0", 0, 0, 1 },
+		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+		  0, 0, 3 },
+	};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if (list[i].bus_kv > 0 && da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].class_kv > 0 && da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		goto cleanup;
+fail:
+		fail = -1;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases
  2021-10-05 14:01   ` David Marchand
@ 2021-10-05 15:56     ` Xueming(Steven) Li
  0 siblings, 0 replies; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-05 15:56 UTC (permalink / raw)
  To: david.marchand; +Cc: NBU-Contact-Thomas Monjalon, dev

On Tue, 2021-10-05 at 16:01 +0200, David Marchand wrote:
> On Tue, Oct 5, 2021 at 2:31 PM Xueming Li <xuemingl@nvidia.com> wrote:
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index f144d8b8ed6..c688ba2b70a 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -42,6 +42,7 @@ test_sources = files(
> >          'test_cryptodev_security_pdcp.c',
> >          'test_cycles.c',
> >          'test_debug.c',
> > +        'test_devargs.c',
> >          'test_distributor.c',
> >          'test_distributor_perf.c',
> >          'test_eal_flags.c',
> 
> You likely want to add devargs_autotest to fast_tests, else "meson
> test" (and the CI) won't execute this new test.
> As for tests referenced in autotest_data.py, I am not sure we really
> need this, but please update too for consistency.

Good suggestion! v1 posted.

> 
> 


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

* Re: [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-19 14:57     ` Gaëtan Rivet
  0 siblings, 0 replies; 43+ messages in thread
From: Gaëtan Rivet @ 2021-10-19 14:57 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev; +Cc: Thomas Monjalon, David Marchand

Hi Steven,

On Tue, Oct 5, 2021, at 17:54, Xueming Li wrote:
> Slash is used to split global device arguments.
>
> To support path value which contains slash, this patch parses devargs by
> locating both slash and layer name key:
>   bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
> "/class=" and "/driver" are valid start of a layer.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

The code looks good to me, and after executing with the test suite it seems
to behave properly.

Reviewed-by: Gaetan Rivet <grive@u256.net>

-- 
Gaetan Rivet

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

* Re: [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-19 15:07     ` Gaëtan Rivet
  2021-10-20  7:20       ` Xueming(Steven) Li
  0 siblings, 1 reply; 43+ messages in thread
From: Gaëtan Rivet @ 2021-10-19 15:07 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev; +Cc: Thomas Monjalon, David Marchand

On Tue, Oct 5, 2021, at 17:54, Xueming Li wrote:
> Initial version to test Global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

Hi,

The test is a very nice addition, absolutely required.
I have however two remarks on the coverage and the implementation, below.

> ---
>  app/test/autotest_data.py |   6 ++
>  app/test/meson.build      |   2 +
>  app/test/test_devargs.c   | 147 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)
>  create mode 100644 app/test/test_devargs.c
>
> diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
> index 302d6374c16..3b841e71918 100644
> --- a/app/test/autotest_data.py
> +++ b/app/test/autotest_data.py
> @@ -785,6 +785,12 @@
>          "Func":    default_autotest,
>          "Report":  None,
>      },
> +    {
> +        "Name":    "Devargs autotest",
> +        "Command": "devargs_autotest",
> +        "Func":    default_autotest,
> +        "Report":  None,
> +    },
>      #
>      # Please always make sure that ring_perf is the last test!
>      #
> diff --git a/app/test/meson.build b/app/test/meson.build
> index f144d8b8ed6..de8540f6119 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -42,6 +42,7 @@ test_sources = files(
>          'test_cryptodev_security_pdcp.c',
>          'test_cycles.c',
>          'test_debug.c',
> +        'test_devargs.c',
>          'test_distributor.c',
>          'test_distributor_perf.c',
>          'test_eal_flags.c',
> @@ -201,6 +202,7 @@ fast_tests = [
>          ['common_autotest', true],
>          ['cpuflags_autotest', true],
>          ['debug_autotest', true],
> +        ['devargs_autotest', true],
>          ['eal_flags_c_opt_autotest', false],
>          ['eal_flags_main_opt_autotest', false],
>          ['eal_flags_n_opt_autotest', false],
> diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
> new file mode 100644
> index 00000000000..8a173368347
> --- /dev/null
> +++ b/app/test/test_devargs.c
> @@ -0,0 +1,147 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2021 NVIDIA Corporation & Affiliates
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include <rte_common.h>
> +#include <rte_devargs.h>
> +#include <rte_kvargs.h>
> +
> +#include "test.h"
> +
> +/* Check layer arguments. */
> +static int
> +test_args(const char *devargs, const char *layer, const char *args, 
> const int n)
> +{
> +	struct rte_kvargs *kvlist;
> +
> +	if (n == 0) {
> +		if (args != NULL && strlen(args) > 0) {
> +			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
> +			       devargs, layer);
> +			return -1;
> +		} else {
> +			return 0;
> +		}
> +	}
> +	if (args == NULL) {
> +		printf("rte_devargs_parse(%s) %s args not parsed\n",
> +		       devargs, layer);
> +		return -1;
> +	}
> +	kvlist = rte_kvargs_parse(args, NULL);
> +	if (kvlist == NULL) {
> +		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
> +		       devargs, layer, args);
> +		return -1;
> +	}
> +	if ((int)kvlist->count != n) {
> +		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
> +		       devargs, layer, args, kvlist->count, n);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/* Test several valid cases */
> +static int
> +test_valid_devargs(void)
> +{
> +	static const struct {
> +		const char *devargs;
> +		int bus_kv;
> +		int class_kv;
> +		int driver_kv;
> +	} list[] = {
> +		/* Global devargs syntax: */
> +		{ "bus=pci", 1, 0, 0 },
> +		{ "class=eth", 0, 1, 0 },
> +		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0", 2, 1, 2 },
> +		{ "bus=vdev,name=/dev/file/name/class=eth", 2, 1, 0 },
> +		/* Legacy devargs syntax: */
> +		{ "1:2.3", 0, 0, 0 },
> +		{ "pci:1:2.3,k0=v0", 0, 0, 1 },
> +		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
> +		  0, 0, 3 },

I would add here cases to verify that edge-case are properly parsed such as:

+               { "bus=vdev,name=/class/bus/path/class=eth", 2, 1, 0 },
[...]
+               { "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+                 0, 0, 3 },

To check the /class or /bus parts cannot throw off the parser (it does not currently).

Additionally, paths with multiple slashes are correct. Maybe add:

+               { "bus=vdev,name=///dblslsh/class=eth", 2, 1, 0 },

as well.

I tested all those cases without issue, it seems the parser is ok.


A second point is that the test only verifies that the numbers of kv were found.
I think this is not robust enough. Instead, I think the 'expect' part of the test
should describe exactly what each layers should be within the devargs (which bus,
class and driver are expected to be found, and the associated string).

Right now the parser could mangle part of a key-value list within the string,
recognize each layers and give incorrect strings to each layer parser.

It might be too much? I don't know. But it seems it could help ensure no error
is introduced at some point by future changes.

-- 
Gaetan Rivet

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

* Re: [dpdk-dev] [PATCH v1 2/3] devargs: make bus key parsing optional
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-19 15:19     ` Gaëtan Rivet
  0 siblings, 0 replies; 43+ messages in thread
From: Gaëtan Rivet @ 2021-10-19 15:19 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev; +Cc: Thomas Monjalon, David Marchand, stable

On Tue, Oct 5, 2021, at 17:54, Xueming Li wrote:
> Global devargs syntax is used as device iteration filter like
> "class=vdpa", a devargs without bus args is valid from parsing
> perspective.
>
> This patch makes bus args optional.
>
> Fixes: d2a66ad79480 ("bus: add device arguments name parsing")
> Cc: stable@dpdk.org

I agree with the change, but I'm not sure it should go into stable.

The question goes further than that: is there a spec describing the valid
syntax to users, that would allow to say the current parsing is buggy?
I haven't found it in the doc.

If there is a change such as this one, it should also be notified in the release notes.
This is user-facing.

So, not saying you should do it and not part of this patch, but a doc might help
in doc/guides/howto maybe. The general grammar of a devargs could be formally
described (in BNF?).

Each layers should document their supported keys as well. Not sure it would be better
gathered in a single file or within each driver.

Now that the global syntax has been enabled for users, it should be documented.

>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

For this patch and beside the stable considerations,
Reviewed-by: Gaetan Rivet <grive@u256.net>

-- 
Gaetan Rivet

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

* Re: [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases
  2021-10-19 15:07     ` Gaëtan Rivet
@ 2021-10-20  7:20       ` Xueming(Steven) Li
  0 siblings, 0 replies; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-20  7:20 UTC (permalink / raw)
  To: dev, grive; +Cc: NBU-Contact-Thomas Monjalon, david.marchand

On Tue, 2021-10-19 at 17:07 +0200, Gaëtan Rivet wrote:
> On Tue, Oct 5, 2021, at 17:54, Xueming Li wrote:
> > Initial version to test Global devargs syntax.
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> 
> Hi,
> 
> The test is a very nice addition, absolutely required.
> I have however two remarks on the coverage and the implementation, below.
> 
> > ---
> >  app/test/autotest_data.py |   6 ++
> >  app/test/meson.build      |   2 +
> >  app/test/test_devargs.c   | 147 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 155 insertions(+)
> >  create mode 100644 app/test/test_devargs.c
> > 
> > diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
> > index 302d6374c16..3b841e71918 100644
> > --- a/app/test/autotest_data.py
> > +++ b/app/test/autotest_data.py
> > @@ -785,6 +785,12 @@
> >          "Func":    default_autotest,
> >          "Report":  None,
> >      },
> > +    {
> > +        "Name":    "Devargs autotest",
> > +        "Command": "devargs_autotest",
> > +        "Func":    default_autotest,
> > +        "Report":  None,
> > +    },
> >      #
> >      # Please always make sure that ring_perf is the last test!
> >      #
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index f144d8b8ed6..de8540f6119 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -42,6 +42,7 @@ test_sources = files(
> >          'test_cryptodev_security_pdcp.c',
> >          'test_cycles.c',
> >          'test_debug.c',
> > +        'test_devargs.c',
> >          'test_distributor.c',
> >          'test_distributor_perf.c',
> >          'test_eal_flags.c',
> > @@ -201,6 +202,7 @@ fast_tests = [
> >          ['common_autotest', true],
> >          ['cpuflags_autotest', true],
> >          ['debug_autotest', true],
> > +        ['devargs_autotest', true],
> >          ['eal_flags_c_opt_autotest', false],
> >          ['eal_flags_main_opt_autotest', false],
> >          ['eal_flags_n_opt_autotest', false],
> > diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
> > new file mode 100644
> > index 00000000000..8a173368347
> > --- /dev/null
> > +++ b/app/test/test_devargs.c
> > @@ -0,0 +1,147 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (c) 2021 NVIDIA Corporation & Affiliates
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include <rte_common.h>
> > +#include <rte_devargs.h>
> > +#include <rte_kvargs.h>
> > +
> > +#include "test.h"
> > +
> > +/* Check layer arguments. */
> > +static int
> > +test_args(const char *devargs, const char *layer, const char *args, 
> > const int n)
> > +{
> > +	struct rte_kvargs *kvlist;
> > +
> > +	if (n == 0) {
> > +		if (args != NULL && strlen(args) > 0) {
> > +			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
> > +			       devargs, layer);
> > +			return -1;
> > +		} else {
> > +			return 0;
> > +		}
> > +	}
> > +	if (args == NULL) {
> > +		printf("rte_devargs_parse(%s) %s args not parsed\n",
> > +		       devargs, layer);
> > +		return -1;
> > +	}
> > +	kvlist = rte_kvargs_parse(args, NULL);
> > +	if (kvlist == NULL) {
> > +		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
> > +		       devargs, layer, args);
> > +		return -1;
> > +	}
> > +	if ((int)kvlist->count != n) {
> > +		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
> > +		       devargs, layer, args, kvlist->count, n);
> > +		return -1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* Test several valid cases */
> > +static int
> > +test_valid_devargs(void)
> > +{
> > +	static const struct {
> > +		const char *devargs;
> > +		int bus_kv;
> > +		int class_kv;
> > +		int driver_kv;
> > +	} list[] = {
> > +		/* Global devargs syntax: */
> > +		{ "bus=pci", 1, 0, 0 },
> > +		{ "class=eth", 0, 1, 0 },
> > +		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0", 2, 1, 2 },
> > +		{ "bus=vdev,name=/dev/file/name/class=eth", 2, 1, 0 },
> > +		/* Legacy devargs syntax: */
> > +		{ "1:2.3", 0, 0, 0 },
> > +		{ "pci:1:2.3,k0=v0", 0, 0, 1 },
> > +		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
> > +		  0, 0, 3 },
> 
> I would add here cases to verify that edge-case are properly parsed such as:
> 
> +               { "bus=vdev,name=/class/bus/path/class=eth", 2, 1, 0 },
> [...]
> +               { "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> +                 0, 0, 3 },
> 
> To check the /class or /bus parts cannot throw off the parser (it does not currently).
> 
> Additionally, paths with multiple slashes are correct. Maybe add:
> 
> +               { "bus=vdev,name=///dblslsh/class=eth", 2, 1, 0 },
> 
> as well.
> 
> I tested all those cases without issue, it seems the parser is ok.
> 
> 
> A second point is that the test only verifies that the numbers of kv were found.
> I think this is not robust enough. Instead, I think the 'expect' part of the test
> should describe exactly what each layers should be within the devargs (which bus,
> class and driver are expected to be found, and the associated string).
> 
> Right now the parser could mangle part of a key-value list within the string,
> recognize each layers and give incorrect strings to each layer parser.
> 
> It might be too much? I don't know. But it seems it could help ensure no error
> is introduced at some point by future changes.
> 

Good suggestion, will added them in v2, thanks!

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

* [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax
  2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
                     ` (2 preceding siblings ...)
  2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-20  7:31   ` Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 1/3] devargs: support path value for global device arguments Xueming Li
                       ` (2 more replies)
  3 siblings, 3 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20  7:31 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

- Support path in global syntax.
- Fix bus name resolving
- Add devargs test cases

v1: initial version
v2:
 - add test cases to test suite
 - add more test cases, verify device name, bus name and class name

Xueming Li (3):
  devargs: support path value for global device arguments
  devargs: make bus key parsing optional
  test/devargs: add devargs test cases

 app/test/autotest_data.py           | 803 ++++++++++++++++++++++++++++
 app/test/meson.build                |   2 +
 app/test/test_devargs.c             | 184 +++++++
 drivers/bus/pci/pci_params.c        |   8 +-
 lib/eal/common/eal_common_devargs.c | 119 ++---
 5 files changed, 1036 insertions(+), 80 deletions(-)
 create mode 100644 app/test/autotest_data.py
 create mode 100644 app/test/test_devargs.c

-- 
2.33.0


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

* [dpdk-dev] [PATCH v2 1/3] devargs: support path value for global device arguments
  2021-10-20  7:31   ` [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20  7:31     ` Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 2/3] devargs: make bus key parsing optional Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases Xueming Li
  2 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20  7:31 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

Slash is used to split global device arguments.

To support path value which contains slash, this patch parses devargs by
locating both slash and layer name key:
  bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
"/class=" and "/driver" are valid start of a layer.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 lib/eal/common/eal_common_devargs.c | 117 ++++++++++------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 411dd6a75f6..d673598032d 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -29,18 +29,6 @@ TAILQ_HEAD(rte_devargs_list, rte_devargs);
 static struct rte_devargs_list devargs_list =
 	TAILQ_HEAD_INITIALIZER(devargs_list);
 
-static size_t
-devargs_layer_count(const char *s)
-{
-	size_t i = s ? 1 : 0;
-
-	while (s != NULL && s[0] != '\0') {
-		i += s[0] == '/';
-		s++;
-	}
-	return i;
-}
-
 /* Resolve devargs name from bus arguments. */
 static int
 devargs_bus_parse_default(struct rte_devargs *devargs,
@@ -77,23 +65,13 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
 	};
 	struct rte_kvargs_pair *kv = NULL;
-	struct rte_class *cls = NULL;
-	struct rte_bus *bus = NULL;
-	const char *s = devstr;
-	size_t nblayer;
-	size_t i = 0;
+	struct rte_kvargs *bus_kvlist = NULL;
+	char *s;
+	size_t nblayer = 0;
+	size_t i;
 	int ret = 0;
 	bool allocated_data = false;
 
-	/* Split each sub-lists. */
-	nblayer = devargs_layer_count(devstr);
-	if (nblayer > RTE_DIM(layers)) {
-		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
-			nblayer);
-		ret = -E2BIG;
-		goto get_out;
-	}
-
 	/* If the devargs points the devstr
 	 * as source data, then it should not allocate
 	 * anything and keep referring only to it.
@@ -106,33 +84,41 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 			goto get_out;
 		}
 		allocated_data = true;
-		s = devargs->data;
 	}
+	s = devargs->data;
 
 	while (s != NULL) {
-		if (i >= RTE_DIM(layers)) {
-			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
-			ret = -EINVAL;
+		if (nblayer > RTE_DIM(layers)) {
+			ret = -E2BIG;
 			goto get_out;
 		}
-		/*
-		 * The last layer is free-form.
-		 * The "driver" key is not required (but accepted).
-		 */
-		if (strncmp(layers[i].key, s, strlen(layers[i].key)) &&
-				i != RTE_DIM(layers) - 1)
-			goto next_layer;
-		layers[i].str = s;
-		layers[i].kvlist = rte_kvargs_parse_delim(s, NULL, "/");
-		if (layers[i].kvlist == NULL) {
+		layers[nblayer].str = s;
+
+		/* Locate next layer starts with valid layer key. */
+		while (s != NULL) {
+			s = strchr(s, '/');
+			if (s == NULL)
+				break;
+			for (i = 0; i < RTE_DIM(layers); i++) {
+				if (strncmp(s + 1, layers[i].key,
+					    strlen(layers[i].key)) == 0) {
+					*s = '\0';
+					break;
+				}
+			}
+			s++;
+			if (i < RTE_DIM(layers))
+				break;
+		}
+
+		layers[nblayer].kvlist = rte_kvargs_parse
+				(layers[nblayer].str, NULL);
+		if (layers[nblayer].kvlist == NULL) {
 			ret = -EINVAL;
 			goto get_out;
 		}
-		s = strchr(s, '/');
-		if (s != NULL)
-			s++;
-next_layer:
-		i++;
+
+		nblayer++;
 	}
 
 	/* Parse each sub-list. */
@@ -143,52 +129,35 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (kv->key == NULL)
 			continue;
 		if (strcmp(kv->key, RTE_DEVARGS_KEY_BUS) == 0) {
-			bus = rte_bus_find_by_name(kv->value);
-			if (bus == NULL) {
+			bus_kvlist = layers[i].kvlist;
+			devargs->bus_str = layers[i].str;
+			devargs->bus = rte_bus_find_by_name(kv->value);
+			if (devargs->bus == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_CLASS) == 0) {
-			cls = rte_class_find_by_name(kv->value);
-			if (cls == NULL) {
+			devargs->cls_str = layers[i].str;
+			devargs->cls = rte_class_find_by_name(kv->value);
+			if (devargs->cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_DRIVER) == 0) {
-			/* Ignore */
+			devargs->drv_str = layers[i].str;
 			continue;
 		}
 	}
 
-	/* Fill devargs fields. */
-	devargs->bus_str = layers[0].str;
-	devargs->cls_str = layers[1].str;
-	devargs->drv_str = layers[2].str;
-	devargs->bus = bus;
-	devargs->cls = cls;
-
-	/* If we own the data, clean up a bit
-	 * the several layers string, to ease
-	 * their parsing afterward.
-	 */
-	if (devargs->data != devstr) {
-		char *s = devargs->data;
-
-		while ((s = strchr(s, '/'))) {
-			*s = '\0';
-			s++;
-		}
-	}
-
 	/* Resolve devargs name. */
-	if (bus != NULL && bus->devargs_parse != NULL)
-		ret = bus->devargs_parse(devargs);
-	else if (layers[0].kvlist != NULL)
-		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
+		ret = devargs->bus->devargs_parse(devargs);
+	else if (bus_kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, bus_kvlist);
 
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
-- 
2.33.0


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

* [dpdk-dev] [PATCH v2 2/3] devargs: make bus key parsing optional
  2021-10-20  7:31   ` [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-20  7:31     ` Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases Xueming Li
  2 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20  7:31 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

Global devargs syntax is used as device iteration filter like
"class=vdpa", a devargs without bus args is valid from parsing
perspective.

This patch makes bus args optional.

Fixes: d2a66ad79480 ("bus: add device arguments name parsing")

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 drivers/bus/pci/pci_params.c        | 8 +++-----
 lib/eal/common/eal_common_devargs.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 21c2e1d0368..60b424b8297 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -87,11 +87,10 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 	struct rte_kvargs *kvargs;
 	const char *addr_str;
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = 0;
 
-	if (da == NULL)
+	if (da == NULL || da->bus_str == NULL)
 		return 0;
-	RTE_ASSERT(da->bus_str != NULL);
 
 	kvargs = rte_kvargs_parse(da->bus_str, NULL);
 	if (kvargs == NULL) {
@@ -103,9 +102,8 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 
 	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
 	if (addr_str == NULL) {
-		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+		RTE_LOG(DEBUG, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
 			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
-		ret = -ENODEV;
 		goto out;
 	}
 
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index d673598032d..8c7650cf6c2 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -39,7 +39,7 @@ devargs_bus_parse_default(struct rte_devargs *devargs,
 	/* Parse devargs name from bus key-value list. */
 	name = rte_kvargs_get(bus_args, "name");
 	if (name == NULL) {
-		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+		RTE_LOG(DEBUG, EAL, "devargs name not found: %s\n",
 			devargs->data);
 		return 0;
 	}
-- 
2.33.0


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

* [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases
  2021-10-20  7:31   ` [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-20  7:31     ` Xueming Li
  2021-10-20  7:38       ` David Marchand
  2 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-20  7:31 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

Initial version to test Global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/autotest_data.py | 803 ++++++++++++++++++++++++++++++++++++++
 app/test/meson.build      |   2 +
 app/test/test_devargs.c   | 184 +++++++++
 3 files changed, 989 insertions(+)
 create mode 100644 app/test/autotest_data.py
 create mode 100644 app/test/test_devargs.c

diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
new file mode 100644
index 00000000000..3b841e71918
--- /dev/null
+++ b/app/test/autotest_data.py
@@ -0,0 +1,803 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2010-2014 Intel Corporation
+
+# Test data for autotests
+
+from autotest_test_funcs import *
+
+# groups of tests that can be run in parallel
+# the grouping has been found largely empirically
+parallel_test_list = [
+    {
+        "Name":    "Timer autotest",
+        "Command": "timer_autotest",
+        "Func":    timer_autotest,
+        "Report":   None,
+    },
+    {
+        "Name":    "Debug autotest",
+        "Command": "debug_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Errno autotest",
+        "Command": "errno_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Meter autotest",
+        "Command": "meter_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Common autotest",
+        "Command": "common_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Resource autotest",
+        "Command": "resource_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Memory autotest",
+        "Command": "memory_autotest",
+        "Func":    memory_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Read/write lock autotest",
+        "Command": "rwlock_autotest",
+        "Func":    rwlock_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Lcores autotest",
+        "Command": "lcores_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Logs autotest",
+        "Command": "logs_autotest",
+        "Func":    logs_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "CPU flags autotest",
+        "Command": "cpuflags_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Version autotest",
+        "Command": "version_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "EAL filesystem autotest",
+        "Command": "eal_fs_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "EAL flags autotest",
+        "Command": "eal_flags_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash autotest",
+        "Command": "hash_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "LPM autotest",
+        "Command": "lpm_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "LPM6 autotest",
+        "Command": "lpm6_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "RIB autotest",
+        "Command": "rib_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "RIB slow autotest",
+        "Command": "rib_slow_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "RIB6 autotest",
+        "Command": "rib6_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "RIB6 slow autotest",
+        "Command": "rib6_slow_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "FIB autotest",
+        "Command": "fib_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "FIB slow autotest",
+        "Command": "fib_slow_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "FIB6 autotest",
+        "Command": "fib6_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "FIB6 slow autotest",
+        "Command": "fib6_slow_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Memcpy autotest",
+        "Command": "memcpy_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Memzone autotest",
+        "Command": "memzone_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "String autotest",
+        "Command": "string_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Malloc autotest",
+        "Command": "malloc_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Multi-process autotest",
+        "Command": "multiprocess_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Mbuf autotest",
+        "Command": "mbuf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Per-lcore autotest",
+        "Command": "per_lcore_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Ring autotest",
+        "Command": "ring_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Spinlock autotest",
+        "Command": "spinlock_autotest",
+        "Func":    spinlock_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Ticketlock autotest",
+        "Command": "ticketlock_autotest",
+        "Func":    ticketlock_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "MCSlock autotest",
+        "Command": "mcslock_autotest",
+        "Func":    mcslock_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Byte order autotest",
+        "Command": "byteorder_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "TAILQ autotest",
+        "Command": "tailq_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Command-line autotest",
+        "Command": "cmdline_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Interrupts autotest",
+        "Command": "interrupt_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Function reentrancy autotest",
+        "Command": "func_reentrancy_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Mempool autotest",
+        "Command": "mempool_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Atomics autotest",
+        "Command": "atomic_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Prefetch autotest",
+        "Command": "prefetch_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Red autotest",
+        "Command": "red_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "PMD ring autotest",
+        "Command": "ring_pmd_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Access list control autotest",
+        "Command": "acl_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Sched autotest",
+        "Command": "sched_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Eventdev selftest octeontx",
+        "Command": "eventdev_selftest_octeontx",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Event ring autotest",
+        "Command": "event_ring_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Table autotest",
+        "Command": "table_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Flow classify autotest",
+        "Command": "flow_classify_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Event eth rx adapter autotest",
+        "Command": "event_eth_rx_adapter_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "User delay",
+        "Command": "user_delay_us",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Rawdev autotest",
+        "Command": "rawdev_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Kvargs autotest",
+        "Command": "kvargs_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Link bonding autotest",
+        "Command": "link_bonding_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Link bonding mode4 autotest",
+        "Command": "link_bonding_mode4_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Link bonding rssconf autotest",
+        "Command": "link_bonding_rssconf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Crc autotest",
+        "Command": "crc_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Distributor autotest",
+        "Command": "distributor_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Reorder autotest",
+        "Command": "reorder_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Barrier autotest",
+        "Command": "barrier_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Bitmap test",
+        "Command": "bitmap_test",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Bitops test",
+        "Command": "bitops_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash multiwriter autotest",
+        "Command": "hash_multiwriter_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Service autotest",
+        "Command": "service_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Timer racecond autotest",
+        "Command": "timer_racecond_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Member autotest",
+        "Command": "member_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":   "Efd_autotest",
+        "Command": "efd_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Thash autotest",
+        "Command": "thash_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash function autotest",
+        "Command": "hash_functions_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev sw mvsam autotest",
+        "Command": "cryptodev_sw_mvsam_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev dpaa2 sec autotest",
+        "Command": "cryptodev_dpaa2_sec_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev dpaa sec autotest",
+        "Command": "cryptodev_dpaa_sec_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev qat autotest",
+        "Command": "cryptodev_qat_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev aesni mb autotest",
+        "Command": "cryptodev_aesni_mb_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev openssl autotest",
+        "Command": "cryptodev_openssl_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev scheduler autotest",
+        "Command": "cryptodev_scheduler_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev aesni gcm autotest",
+        "Command": "cryptodev_aesni_gcm_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev null autotest",
+        "Command": "cryptodev_null_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev sw snow3g autotest",
+        "Command": "cryptodev_sw_snow3g_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev sw kasumi autotest",
+        "Command": "cryptodev_sw_kasumi_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Cryptodev_sw_zuc_autotest",
+        "Command": "cryptodev_sw_zuc_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Reciprocal division",
+        "Command": "reciprocal_division",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Red all",
+        "Command": "red_all",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+	"Name":    "Fbarray autotest",
+	"Command": "fbarray_autotest",
+	"Func":    default_autotest,
+	"Report":  None,
+    },
+    {
+	"Name":    "External memory autotest",
+	"Command": "external_mem_autotest",
+	"Func":    default_autotest,
+	"Report":  None,
+    },
+    {
+        "Name":    "Metrics autotest",
+        "Command": "metrics_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Bitratestats autotest",
+        "Command": "bitratestats_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Latencystats autotest",
+        "Command": "latencystats_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Pdump autotest",
+        "Command": "pdump_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "IPsec_SAD",
+        "Command": "ipsec_sad_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Checksum autotest",
+        "Command": "cksum_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    #
+    #Please always keep all dump tests at the end and together!
+    #
+    {
+        "Name":    "Dump physmem",
+        "Command": "dump_physmem",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump memzone",
+        "Command": "dump_memzone",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump struct sizes",
+        "Command": "dump_struct_sizes",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump mempool",
+        "Command": "dump_mempool",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump malloc stats",
+        "Command": "dump_malloc_stats",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump devargs",
+        "Command": "dump_devargs",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump log types",
+        "Command": "dump_log_types",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Dump_ring",
+        "Command": "dump_ring",
+        "Func":    dump_autotest,
+        "Report":  None,
+    },
+]
+
+# tests that should not be run when any other tests are running
+non_parallel_test_list = [
+    {
+        "Name":    "Eventdev common autotest",
+        "Command": "eventdev_common_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Eventdev selftest sw",
+        "Command": "eventdev_selftest_sw",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "KNI autotest",
+        "Command": "kni_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Mempool performance autotest",
+        "Command": "mempool_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Memcpy performance autotest",
+        "Command": "memcpy_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash performance autotest",
+        "Command": "hash_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash read-write concurrency functional autotest",
+        "Command": "hash_readwrite_func_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash read-write concurrency perf autotest",
+        "Command": "hash_readwrite_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Hash read-write lock-free concurrency perf autotest",
+        "Command": "hash_readwrite_lf_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":       "Power autotest",
+        "Command":    "power_autotest",
+        "Func":       default_autotest,
+        "Report":      None,
+    },
+    {
+        "Name":       "Power cpufreq autotest",
+        "Command":    "power_cpufreq_autotest",
+        "Func":       default_autotest,
+        "Report":     None,
+    },
+    {
+        "Name":       "Power KVM VM  autotest",
+        "Command":    "power_kvm_vm_autotest",
+        "Func":       default_autotest,
+        "Report":     None,
+    },
+    {
+        "Name":    "Timer performance autotest",
+        "Command": "timer_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+
+        "Name":    "Pmd perf autotest",
+        "Command": "pmd_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Ring pmd perf autotest",
+        "Command": "ring_pmd_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Distributor perf autotest",
+        "Command": "distributor_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Red_perf",
+        "Command": "red_perf",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Lpm6 perf autotest",
+        "Command": "lpm6_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Lpm perf autotest",
+        "Command": "lpm_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "FIB perf autotest",
+        "Command": "fib_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "FIB6 perf autotest",
+        "Command": "fib6_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+         "Name":    "Efd perf autotest",
+         "Command": "efd_perf_autotest",
+         "Func":    default_autotest,
+         "Report":  None,
+    },
+    {
+        "Name":    "Member perf autotest",
+        "Command": "member_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Reciprocal division perf",
+        "Command": "reciprocal_division_perf",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "RCU QSBR autotest",
+        "Command": "rcu_qsbr_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "RCU QSBR performance autotest",
+        "Command": "rcu_qsbr_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    {
+        "Name":    "Devargs autotest",
+        "Command": "devargs_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+    #
+    # Please always make sure that ring_perf is the last test!
+    #
+    {
+        "Name":    "Ring performance autotest",
+        "Command": "ring_perf_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
+]
diff --git a/app/test/meson.build b/app/test/meson.build
index a16374b7a10..842388b0d32 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -42,6 +42,7 @@ test_sources = files(
         'test_cryptodev_security_pdcp.c',
         'test_cycles.c',
         'test_debug.c',
+        'test_devargs.c',
         'test_distributor.c',
         'test_distributor_perf.c',
         'test_dmadev.c',
@@ -204,6 +205,7 @@ fast_tests = [
         ['common_autotest', true],
         ['cpuflags_autotest', true],
         ['debug_autotest', true],
+        ['devargs_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
         ['eal_flags_n_opt_autotest', false],
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..13e95f052b0
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+#include <rte_bus.h>
+#include <rte_class.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct {
+		const char *devargs;
+		int bus_kv;
+		int class_kv;
+		int driver_kv;
+		const char *bus;
+		const char *name;
+		const char *class;
+	} list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci",
+		  1, 0, 0, "pci", NULL, NULL},
+		{ "class=eth",
+		  0, 1, 0, NULL, NULL, "eth" },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0",
+		  2, 1, 2, "pci", "0000:01:02.3", "eth" },
+		{ "bus=vdev,name=/dev/file/name/class=eth",
+		  2, 1, 0, "vdev", "/dev/file/name", "eth" },
+		{ "bus=vdev,name=/class/bus/path/class=eth",
+		  2, 1, 0, "vdev", "/class/bus/path", "eth" },
+		{ "bus=vdev,name=///dblslsh/class=eth",
+		  2, 1, 0, "vdev", "///dblslsh", "eth" },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0,
+		  "pci", "1:2.3", NULL },
+		{ "pci:1:2.3,k0=v0",
+		  0, 0, 1, "pci", "1:2.3", NULL },
+		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+		{ "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+	};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if ((list[i].bus_kv > 0 || list[i].bus != NULL) &&
+		    da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].bus != NULL &&
+		    strcmp(da.bus->name, list[i].bus) != 0) {
+			printf("rte_devargs_parse(%s) bus name (%s) not expected (%s)\n",
+			       list[i].devargs, da.bus->name, list[i].bus);
+			goto fail;
+		}
+		if ((list[i].class_kv > 0 || list[i].class != NULL) &&
+		    da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (list[i].class != NULL &&
+		    strcmp(da.cls->name, list[i].class) != 0) {
+			printf("rte_devargs_parse(%s) class name (%s) not expected (%s)\n",
+			       list[i].devargs, da.cls->name, list[i].class);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		if (list[i].name != NULL &&
+		    strcmp(da.name, list[i].name) != 0) {
+			printf("rte_devargs_parse(%s) device name (%s) not expected (%s)\n",
+			       list[i].devargs, da.name, list[i].name);
+			goto fail;
+		}
+		goto cleanup;
+fail:
+		fail = -1;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases
  2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-20  7:38       ` David Marchand
  2021-10-20  8:23         ` Xueming(Steven) Li
  0 siblings, 1 reply; 43+ messages in thread
From: David Marchand @ 2021-10-20  7:38 UTC (permalink / raw)
  To: Xueming Li; +Cc: dev, Gaetan Rivet, Thomas Monjalon, Lior Margalit

On Wed, Oct 20, 2021 at 9:32 AM Xueming Li <xuemingl@nvidia.com> wrote:
>
> Initial version to test Global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  app/test/autotest_data.py | 803 ++++++++++++++++++++++++++++++++++++++

This file is getting reintroduced by your patch.
We dropped it recently:
https://git.dpdk.org/dpdk/commit/?id=8c745bb62340e7b6a3ad61e5d79dfceebd4c28e4


>  app/test/meson.build      |   2 +
>  app/test/test_devargs.c   | 184 +++++++++
>  3 files changed, 989 insertions(+)
>  create mode 100644 app/test/autotest_data.py
>  create mode 100644 app/test/test_devargs.c
>


-- 
David Marchand


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

* [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax
  2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
                   ` (2 preceding siblings ...)
  2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20  8:21 ` Xueming Li
  2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 1/3] devargs: support path value for global device arguments Xueming Li
                     ` (2 more replies)
  2021-10-20 11:12 ` [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
  5 siblings, 3 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20  8:21 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

- Support path in global syntax.
- Fix bus name resolving
- Add devargs test cases

v1: initial version
v2:
 - add test cases to test suite
 - add more test cases, verify device name, bus name and class name
v3:
 - remove autotest_data.py

Xueming Li (3):
  devargs: support path value for global device arguments
  devargs: make bus key parsing optional
  test/devargs: add devargs test cases

 app/test/meson.build                |   2 +
 app/test/test_devargs.c             | 184 ++++++++++++++++++++++++++++
 drivers/bus/pci/pci_params.c        |   8 +-
 lib/eal/common/eal_common_devargs.c | 119 +++++++-----------
 4 files changed, 233 insertions(+), 80 deletions(-)
 create mode 100644 app/test/test_devargs.c

-- 
2.33.0


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

* [dpdk-dev] [PATCH v3 1/3] devargs: support path value for global device arguments
  2021-10-20  8:21 ` [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20  8:21   ` Xueming Li
  2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 2/3] devargs: make bus key parsing optional Xueming Li
  2021-10-20  8:22   ` [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases Xueming Li
  2 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20  8:21 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

Slash is used to split global device arguments.

To support path value which contains slash, this patch parses devargs by
locating both slash and layer name key:
  bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
"/class=" and "/driver" are valid start of a layer.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 lib/eal/common/eal_common_devargs.c | 117 ++++++++++------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 411dd6a75f6..d673598032d 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -29,18 +29,6 @@ TAILQ_HEAD(rte_devargs_list, rte_devargs);
 static struct rte_devargs_list devargs_list =
 	TAILQ_HEAD_INITIALIZER(devargs_list);
 
-static size_t
-devargs_layer_count(const char *s)
-{
-	size_t i = s ? 1 : 0;
-
-	while (s != NULL && s[0] != '\0') {
-		i += s[0] == '/';
-		s++;
-	}
-	return i;
-}
-
 /* Resolve devargs name from bus arguments. */
 static int
 devargs_bus_parse_default(struct rte_devargs *devargs,
@@ -77,23 +65,13 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
 	};
 	struct rte_kvargs_pair *kv = NULL;
-	struct rte_class *cls = NULL;
-	struct rte_bus *bus = NULL;
-	const char *s = devstr;
-	size_t nblayer;
-	size_t i = 0;
+	struct rte_kvargs *bus_kvlist = NULL;
+	char *s;
+	size_t nblayer = 0;
+	size_t i;
 	int ret = 0;
 	bool allocated_data = false;
 
-	/* Split each sub-lists. */
-	nblayer = devargs_layer_count(devstr);
-	if (nblayer > RTE_DIM(layers)) {
-		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
-			nblayer);
-		ret = -E2BIG;
-		goto get_out;
-	}
-
 	/* If the devargs points the devstr
 	 * as source data, then it should not allocate
 	 * anything and keep referring only to it.
@@ -106,33 +84,41 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 			goto get_out;
 		}
 		allocated_data = true;
-		s = devargs->data;
 	}
+	s = devargs->data;
 
 	while (s != NULL) {
-		if (i >= RTE_DIM(layers)) {
-			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
-			ret = -EINVAL;
+		if (nblayer > RTE_DIM(layers)) {
+			ret = -E2BIG;
 			goto get_out;
 		}
-		/*
-		 * The last layer is free-form.
-		 * The "driver" key is not required (but accepted).
-		 */
-		if (strncmp(layers[i].key, s, strlen(layers[i].key)) &&
-				i != RTE_DIM(layers) - 1)
-			goto next_layer;
-		layers[i].str = s;
-		layers[i].kvlist = rte_kvargs_parse_delim(s, NULL, "/");
-		if (layers[i].kvlist == NULL) {
+		layers[nblayer].str = s;
+
+		/* Locate next layer starts with valid layer key. */
+		while (s != NULL) {
+			s = strchr(s, '/');
+			if (s == NULL)
+				break;
+			for (i = 0; i < RTE_DIM(layers); i++) {
+				if (strncmp(s + 1, layers[i].key,
+					    strlen(layers[i].key)) == 0) {
+					*s = '\0';
+					break;
+				}
+			}
+			s++;
+			if (i < RTE_DIM(layers))
+				break;
+		}
+
+		layers[nblayer].kvlist = rte_kvargs_parse
+				(layers[nblayer].str, NULL);
+		if (layers[nblayer].kvlist == NULL) {
 			ret = -EINVAL;
 			goto get_out;
 		}
-		s = strchr(s, '/');
-		if (s != NULL)
-			s++;
-next_layer:
-		i++;
+
+		nblayer++;
 	}
 
 	/* Parse each sub-list. */
@@ -143,52 +129,35 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (kv->key == NULL)
 			continue;
 		if (strcmp(kv->key, RTE_DEVARGS_KEY_BUS) == 0) {
-			bus = rte_bus_find_by_name(kv->value);
-			if (bus == NULL) {
+			bus_kvlist = layers[i].kvlist;
+			devargs->bus_str = layers[i].str;
+			devargs->bus = rte_bus_find_by_name(kv->value);
+			if (devargs->bus == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_CLASS) == 0) {
-			cls = rte_class_find_by_name(kv->value);
-			if (cls == NULL) {
+			devargs->cls_str = layers[i].str;
+			devargs->cls = rte_class_find_by_name(kv->value);
+			if (devargs->cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_DRIVER) == 0) {
-			/* Ignore */
+			devargs->drv_str = layers[i].str;
 			continue;
 		}
 	}
 
-	/* Fill devargs fields. */
-	devargs->bus_str = layers[0].str;
-	devargs->cls_str = layers[1].str;
-	devargs->drv_str = layers[2].str;
-	devargs->bus = bus;
-	devargs->cls = cls;
-
-	/* If we own the data, clean up a bit
-	 * the several layers string, to ease
-	 * their parsing afterward.
-	 */
-	if (devargs->data != devstr) {
-		char *s = devargs->data;
-
-		while ((s = strchr(s, '/'))) {
-			*s = '\0';
-			s++;
-		}
-	}
-
 	/* Resolve devargs name. */
-	if (bus != NULL && bus->devargs_parse != NULL)
-		ret = bus->devargs_parse(devargs);
-	else if (layers[0].kvlist != NULL)
-		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
+		ret = devargs->bus->devargs_parse(devargs);
+	else if (bus_kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, bus_kvlist);
 
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
-- 
2.33.0


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

* [dpdk-dev] [PATCH v3 2/3] devargs: make bus key parsing optional
  2021-10-20  8:21 ` [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-20  8:21   ` Xueming Li
  2021-10-20  8:22   ` [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases Xueming Li
  2 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20  8:21 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

Global devargs syntax is used as device iteration filter like
"class=vdpa", a devargs without bus args is valid from parsing
perspective.

This patch makes bus args optional.

Fixes: d2a66ad79480 ("bus: add device arguments name parsing")

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 drivers/bus/pci/pci_params.c        | 8 +++-----
 lib/eal/common/eal_common_devargs.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 21c2e1d0368..60b424b8297 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -87,11 +87,10 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 	struct rte_kvargs *kvargs;
 	const char *addr_str;
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = 0;
 
-	if (da == NULL)
+	if (da == NULL || da->bus_str == NULL)
 		return 0;
-	RTE_ASSERT(da->bus_str != NULL);
 
 	kvargs = rte_kvargs_parse(da->bus_str, NULL);
 	if (kvargs == NULL) {
@@ -103,9 +102,8 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 
 	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
 	if (addr_str == NULL) {
-		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+		RTE_LOG(DEBUG, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
 			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
-		ret = -ENODEV;
 		goto out;
 	}
 
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index d673598032d..8c7650cf6c2 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -39,7 +39,7 @@ devargs_bus_parse_default(struct rte_devargs *devargs,
 	/* Parse devargs name from bus key-value list. */
 	name = rte_kvargs_get(bus_args, "name");
 	if (name == NULL) {
-		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+		RTE_LOG(DEBUG, EAL, "devargs name not found: %s\n",
 			devargs->data);
 		return 0;
 	}
-- 
2.33.0


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

* [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases
  2021-10-20  8:21 ` [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-20  8:22   ` Xueming Li
  2021-10-20  9:08     ` David Marchand
  2 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-20  8:22 UTC (permalink / raw)
  To: dev, Gaetan Rivet; +Cc: xuemingl, Thomas Monjalon, Lior Margalit

Initial version to test Global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/meson.build    |   2 +
 app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 186 insertions(+)
 create mode 100644 app/test/test_devargs.c

diff --git a/app/test/meson.build b/app/test/meson.build
index a16374b7a10..842388b0d32 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -42,6 +42,7 @@ test_sources = files(
         'test_cryptodev_security_pdcp.c',
         'test_cycles.c',
         'test_debug.c',
+        'test_devargs.c',
         'test_distributor.c',
         'test_distributor_perf.c',
         'test_dmadev.c',
@@ -204,6 +205,7 @@ fast_tests = [
         ['common_autotest', true],
         ['cpuflags_autotest', true],
         ['debug_autotest', true],
+        ['devargs_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
         ['eal_flags_n_opt_autotest', false],
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..13e95f052b0
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+#include <rte_bus.h>
+#include <rte_class.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct {
+		const char *devargs;
+		int bus_kv;
+		int class_kv;
+		int driver_kv;
+		const char *bus;
+		const char *name;
+		const char *class;
+	} list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci",
+		  1, 0, 0, "pci", NULL, NULL},
+		{ "class=eth",
+		  0, 1, 0, NULL, NULL, "eth" },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0",
+		  2, 1, 2, "pci", "0000:01:02.3", "eth" },
+		{ "bus=vdev,name=/dev/file/name/class=eth",
+		  2, 1, 0, "vdev", "/dev/file/name", "eth" },
+		{ "bus=vdev,name=/class/bus/path/class=eth",
+		  2, 1, 0, "vdev", "/class/bus/path", "eth" },
+		{ "bus=vdev,name=///dblslsh/class=eth",
+		  2, 1, 0, "vdev", "///dblslsh", "eth" },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0,
+		  "pci", "1:2.3", NULL },
+		{ "pci:1:2.3,k0=v0",
+		  0, 0, 1, "pci", "1:2.3", NULL },
+		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+		{ "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+	};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if ((list[i].bus_kv > 0 || list[i].bus != NULL) &&
+		    da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].bus != NULL &&
+		    strcmp(da.bus->name, list[i].bus) != 0) {
+			printf("rte_devargs_parse(%s) bus name (%s) not expected (%s)\n",
+			       list[i].devargs, da.bus->name, list[i].bus);
+			goto fail;
+		}
+		if ((list[i].class_kv > 0 || list[i].class != NULL) &&
+		    da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (list[i].class != NULL &&
+		    strcmp(da.cls->name, list[i].class) != 0) {
+			printf("rte_devargs_parse(%s) class name (%s) not expected (%s)\n",
+			       list[i].devargs, da.cls->name, list[i].class);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		if (list[i].name != NULL &&
+		    strcmp(da.name, list[i].name) != 0) {
+			printf("rte_devargs_parse(%s) device name (%s) not expected (%s)\n",
+			       list[i].devargs, da.name, list[i].name);
+			goto fail;
+		}
+		goto cleanup;
+fail:
+		fail = -1;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases
  2021-10-20  7:38       ` David Marchand
@ 2021-10-20  8:23         ` Xueming(Steven) Li
  0 siblings, 0 replies; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-20  8:23 UTC (permalink / raw)
  To: david.marchand; +Cc: Lior Margalit, NBU-Contact-Thomas Monjalon, dev, grive

On Wed, 2021-10-20 at 09:38 +0200, David Marchand wrote:
> On Wed, Oct 20, 2021 at 9:32 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > 
> > Initial version to test Global devargs syntax.
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> >  app/test/autotest_data.py | 803 ++++++++++++++++++++++++++++++++++++++
> 
> This file is getting reintroduced by your patch.
> We dropped it recently:
> https://git.dpdk.org/dpdk/commit/?id=8c745bb62340e7b6a3ad61e5d79dfceebd4c28e4

My bad, forgot to remove it :) new version posted.

> 
> 
> >  app/test/meson.build      |   2 +
> >  app/test/test_devargs.c   | 184 +++++++++
> >  3 files changed, 989 insertions(+)
> >  create mode 100644 app/test/autotest_data.py
> >  create mode 100644 app/test/test_devargs.c
> > 
> 
> 


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

* Re: [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases
  2021-10-20  8:22   ` [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-20  9:08     ` David Marchand
  2021-10-20  9:40       ` Xueming(Steven) Li
  0 siblings, 1 reply; 43+ messages in thread
From: David Marchand @ 2021-10-20  9:08 UTC (permalink / raw)
  To: Xueming Li, Thomas Monjalon; +Cc: dev, Gaetan Rivet, Lior Margalit

On Wed, Oct 20, 2021 at 10:22 AM Xueming Li <xuemingl@nvidia.com> wrote:
>
> Initial version to test Global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

10/95 DPDK:fast-tests / devargs_autotest      FAIL     0.17 s (exit
status 255 or signal 127 SIGinvalid)

--- command ---
DPDK_TEST='devargs_autotest'
/home/runner/work/dpdk/dpdk/build/app/test/dpdk-test -l 0-1
--file-prefix=devargs_autotest
--- stdout ---
RTE>>devargs_autotest
== test valid case ==
rte_devargs_parse(net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1)
returned -14 (but should not)
rte_devargs_parse(net_virtio_user0,iface=test,path=/class/bus/,queues=1)
returned -14 (but should not)
Test Failed
RTE>>
--- stderr ---
EAL: Detected CPU lcores: 2
EAL: Detected NUMA nodes: 1
EAL: Detected shared linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/devargs_autotest/mp_socket
EAL: Selected IOVA mode 'PA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
APP: HPET is not enabled, using TSC as default timer
EAL: failed to parse device "net_virtio_user0"
EAL: failed to parse device "net_virtio_user0"
-------


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases
  2021-10-20  9:08     ` David Marchand
@ 2021-10-20  9:40       ` Xueming(Steven) Li
  2021-10-20 10:41         ` Bruce Richardson
  0 siblings, 1 reply; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-20  9:40 UTC (permalink / raw)
  To: NBU-Contact-Thomas Monjalon, david.marchand; +Cc: Lior Margalit, dev, grive

On Wed, 2021-10-20 at 11:08 +0200, David Marchand wrote:
> On Wed, Oct 20, 2021 at 10:22 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > 
> > Initial version to test Global devargs syntax.
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> 
> 10/95 DPDK:fast-tests / devargs_autotest      FAIL     0.17 s (exit
> status 255 or signal 127 SIGinvalid)
> 
> --- command ---
> DPDK_TEST='devargs_autotest'
> /home/runner/work/dpdk/dpdk/build/app/test/dpdk-test -l 0-1
> --file-prefix=devargs_autotest
> --- stdout ---
> RTE>>devargs_autotest
> == test valid case ==
> rte_devargs_parse(net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1)
> returned -14 (but should not)
> rte_devargs_parse(net_virtio_user0,iface=test,path=/class/bus/,queues=1)
> returned -14 (but should not)
> Test Failed
> RTE>>
> --- stderr ---
> EAL: Detected CPU lcores: 2
> EAL: Detected NUMA nodes: 1
> EAL: Detected shared linkage of DPDK
> EAL: Multi-process socket /var/run/dpdk/devargs_autotest/mp_socket
> EAL: Selected IOVA mode 'PA'
> EAL: No available 1048576 kB hugepages reported
> EAL: VFIO support initialized
> APP: HPET is not enabled, using TSC as default timer
> EAL: failed to parse device "net_virtio_user0"
> EAL: failed to parse device "net_virtio_user0"

Yes, noticed that, seems virtio driver not enabled. Tried to add
"net_virtio" to test_deps in meson file, but failed with:
../app/test/meson.build:444:4: ERROR: Tried to get unknown variable
"static_rte_net_virtio".

Seems meson scripts only lookup dependency on libs, I'm not good at
meson, any suggestion? 

> -------
> 
> 


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

* Re: [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases
  2021-10-20  9:40       ` Xueming(Steven) Li
@ 2021-10-20 10:41         ` Bruce Richardson
  0 siblings, 0 replies; 43+ messages in thread
From: Bruce Richardson @ 2021-10-20 10:41 UTC (permalink / raw)
  To: Xueming(Steven) Li
  Cc: NBU-Contact-Thomas Monjalon, david.marchand, Lior Margalit, dev, grive

On Wed, Oct 20, 2021 at 09:40:55AM +0000, Xueming(Steven) Li wrote:
> On Wed, 2021-10-20 at 11:08 +0200, David Marchand wrote:
> > On Wed, Oct 20, 2021 at 10:22 AM Xueming Li <xuemingl@nvidia.com> wrote:
> > > 
> > > Initial version to test Global devargs syntax.
> > > 
> > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > 
> > 10/95 DPDK:fast-tests / devargs_autotest      FAIL     0.17 s (exit
> > status 255 or signal 127 SIGinvalid)
> > 
> > --- command ---
> > DPDK_TEST='devargs_autotest'
> > /home/runner/work/dpdk/dpdk/build/app/test/dpdk-test -l 0-1
> > --file-prefix=devargs_autotest
> > --- stdout ---
> > RTE>>devargs_autotest
> > == test valid case ==
> > rte_devargs_parse(net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1)
> > returned -14 (but should not)
> > rte_devargs_parse(net_virtio_user0,iface=test,path=/class/bus/,queues=1)
> > returned -14 (but should not)
> > Test Failed
> > RTE>>
> > --- stderr ---
> > EAL: Detected CPU lcores: 2
> > EAL: Detected NUMA nodes: 1
> > EAL: Detected shared linkage of DPDK
> > EAL: Multi-process socket /var/run/dpdk/devargs_autotest/mp_socket
> > EAL: Selected IOVA mode 'PA'
> > EAL: No available 1048576 kB hugepages reported
> > EAL: VFIO support initialized
> > APP: HPET is not enabled, using TSC as default timer
> > EAL: failed to parse device "net_virtio_user0"
> > EAL: failed to parse device "net_virtio_user0"
> 
> Yes, noticed that, seems virtio driver not enabled. Tried to add
> "net_virtio" to test_deps in meson file, but failed with:
> ../app/test/meson.build:444:4: ERROR: Tried to get unknown variable
> "static_rte_net_virtio".
> 
> Seems meson scripts only lookup dependency on libs, I'm not good at
> meson, any suggestion? 
>
"net_virtio" is correct for the virtio driver. However, you probably need
to do like is done for the other network drivers linked into the autotest,
and make it conditional on the support being present, since not every build
enables the virtio driver. For example, as is currently in the meson.build
for the autotests:

# The following linkages of drivers are required because
# they are used via a driver-specific API.
if dpdk_conf.has('RTE_NET_BOND')
    test_deps += 'net_bond'
    test_sources += ['test_link_bonding.c', 'test_link_bonding_rssconf.c']
    ...
endif

/Bruce

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

* [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax
  2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
                   ` (3 preceding siblings ...)
  2021-10-20  8:21 ` [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20 11:12 ` Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 1/3] devargs: support path value for global device arguments Xueming Li
                     ` (2 more replies)
  2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
  5 siblings, 3 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 11:12 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

- Support path in global syntax.
- Fix bus name resolving
- Add devargs test cases

v1: initial version
v2:
 - add test cases to test suite
 - add more test cases, verify device name, bus name and class name
v3:
 - remove autotest_data.py
v4:
 - make devargs test depends on virtio driver

Email thread: 20211005123012.264727-1-xuemingl@nvidia.com

Xueming Li (3):
  devargs: support path value for global device arguments
  devargs: make bus key parsing optional
  test/devargs: add devargs test cases

 app/test/meson.build                |   5 +
 app/test/test_devargs.c             | 184 ++++++++++++++++++++++++++++
 drivers/bus/pci/pci_params.c        |   8 +-
 lib/eal/common/eal_common_devargs.c | 119 +++++++-----------
 4 files changed, 236 insertions(+), 80 deletions(-)
 create mode 100644 app/test/test_devargs.c

-- 
2.33.0


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

* [dpdk-dev] [PATCH v4 1/3] devargs: support path value for global device arguments
  2021-10-20 11:12 ` [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20 11:12   ` Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 2/3] devargs: make bus key parsing optional Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases Xueming Li
  2 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 11:12 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

Slash is used to split global device arguments.

To support path value which contains slash, this patch parses devargs by
locating both slash and layer name key:
  bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
"/class=" and "/driver" are valid start of a layer.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 lib/eal/common/eal_common_devargs.c | 117 ++++++++++------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 411dd6a75f6..d673598032d 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -29,18 +29,6 @@ TAILQ_HEAD(rte_devargs_list, rte_devargs);
 static struct rte_devargs_list devargs_list =
 	TAILQ_HEAD_INITIALIZER(devargs_list);
 
-static size_t
-devargs_layer_count(const char *s)
-{
-	size_t i = s ? 1 : 0;
-
-	while (s != NULL && s[0] != '\0') {
-		i += s[0] == '/';
-		s++;
-	}
-	return i;
-}
-
 /* Resolve devargs name from bus arguments. */
 static int
 devargs_bus_parse_default(struct rte_devargs *devargs,
@@ -77,23 +65,13 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
 	};
 	struct rte_kvargs_pair *kv = NULL;
-	struct rte_class *cls = NULL;
-	struct rte_bus *bus = NULL;
-	const char *s = devstr;
-	size_t nblayer;
-	size_t i = 0;
+	struct rte_kvargs *bus_kvlist = NULL;
+	char *s;
+	size_t nblayer = 0;
+	size_t i;
 	int ret = 0;
 	bool allocated_data = false;
 
-	/* Split each sub-lists. */
-	nblayer = devargs_layer_count(devstr);
-	if (nblayer > RTE_DIM(layers)) {
-		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
-			nblayer);
-		ret = -E2BIG;
-		goto get_out;
-	}
-
 	/* If the devargs points the devstr
 	 * as source data, then it should not allocate
 	 * anything and keep referring only to it.
@@ -106,33 +84,41 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 			goto get_out;
 		}
 		allocated_data = true;
-		s = devargs->data;
 	}
+	s = devargs->data;
 
 	while (s != NULL) {
-		if (i >= RTE_DIM(layers)) {
-			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
-			ret = -EINVAL;
+		if (nblayer > RTE_DIM(layers)) {
+			ret = -E2BIG;
 			goto get_out;
 		}
-		/*
-		 * The last layer is free-form.
-		 * The "driver" key is not required (but accepted).
-		 */
-		if (strncmp(layers[i].key, s, strlen(layers[i].key)) &&
-				i != RTE_DIM(layers) - 1)
-			goto next_layer;
-		layers[i].str = s;
-		layers[i].kvlist = rte_kvargs_parse_delim(s, NULL, "/");
-		if (layers[i].kvlist == NULL) {
+		layers[nblayer].str = s;
+
+		/* Locate next layer starts with valid layer key. */
+		while (s != NULL) {
+			s = strchr(s, '/');
+			if (s == NULL)
+				break;
+			for (i = 0; i < RTE_DIM(layers); i++) {
+				if (strncmp(s + 1, layers[i].key,
+					    strlen(layers[i].key)) == 0) {
+					*s = '\0';
+					break;
+				}
+			}
+			s++;
+			if (i < RTE_DIM(layers))
+				break;
+		}
+
+		layers[nblayer].kvlist = rte_kvargs_parse
+				(layers[nblayer].str, NULL);
+		if (layers[nblayer].kvlist == NULL) {
 			ret = -EINVAL;
 			goto get_out;
 		}
-		s = strchr(s, '/');
-		if (s != NULL)
-			s++;
-next_layer:
-		i++;
+
+		nblayer++;
 	}
 
 	/* Parse each sub-list. */
@@ -143,52 +129,35 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (kv->key == NULL)
 			continue;
 		if (strcmp(kv->key, RTE_DEVARGS_KEY_BUS) == 0) {
-			bus = rte_bus_find_by_name(kv->value);
-			if (bus == NULL) {
+			bus_kvlist = layers[i].kvlist;
+			devargs->bus_str = layers[i].str;
+			devargs->bus = rte_bus_find_by_name(kv->value);
+			if (devargs->bus == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_CLASS) == 0) {
-			cls = rte_class_find_by_name(kv->value);
-			if (cls == NULL) {
+			devargs->cls_str = layers[i].str;
+			devargs->cls = rte_class_find_by_name(kv->value);
+			if (devargs->cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_DRIVER) == 0) {
-			/* Ignore */
+			devargs->drv_str = layers[i].str;
 			continue;
 		}
 	}
 
-	/* Fill devargs fields. */
-	devargs->bus_str = layers[0].str;
-	devargs->cls_str = layers[1].str;
-	devargs->drv_str = layers[2].str;
-	devargs->bus = bus;
-	devargs->cls = cls;
-
-	/* If we own the data, clean up a bit
-	 * the several layers string, to ease
-	 * their parsing afterward.
-	 */
-	if (devargs->data != devstr) {
-		char *s = devargs->data;
-
-		while ((s = strchr(s, '/'))) {
-			*s = '\0';
-			s++;
-		}
-	}
-
 	/* Resolve devargs name. */
-	if (bus != NULL && bus->devargs_parse != NULL)
-		ret = bus->devargs_parse(devargs);
-	else if (layers[0].kvlist != NULL)
-		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
+		ret = devargs->bus->devargs_parse(devargs);
+	else if (bus_kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, bus_kvlist);
 
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
-- 
2.33.0


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

* [dpdk-dev] [PATCH v4 2/3] devargs: make bus key parsing optional
  2021-10-20 11:12 ` [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-20 11:12   ` Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases Xueming Li
  2 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 11:12 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

Global devargs syntax is used as device iteration filter like
"class=vdpa", a devargs without bus args is valid from parsing
perspective.

This patch makes bus args optional.

Fixes: d2a66ad79480 ("bus: add device arguments name parsing")

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 drivers/bus/pci/pci_params.c        | 8 +++-----
 lib/eal/common/eal_common_devargs.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 21c2e1d0368..60b424b8297 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -87,11 +87,10 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 	struct rte_kvargs *kvargs;
 	const char *addr_str;
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = 0;
 
-	if (da == NULL)
+	if (da == NULL || da->bus_str == NULL)
 		return 0;
-	RTE_ASSERT(da->bus_str != NULL);
 
 	kvargs = rte_kvargs_parse(da->bus_str, NULL);
 	if (kvargs == NULL) {
@@ -103,9 +102,8 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 
 	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
 	if (addr_str == NULL) {
-		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+		RTE_LOG(DEBUG, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
 			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
-		ret = -ENODEV;
 		goto out;
 	}
 
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index d673598032d..8c7650cf6c2 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -39,7 +39,7 @@ devargs_bus_parse_default(struct rte_devargs *devargs,
 	/* Parse devargs name from bus key-value list. */
 	name = rte_kvargs_get(bus_args, "name");
 	if (name == NULL) {
-		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+		RTE_LOG(DEBUG, EAL, "devargs name not found: %s\n",
 			devargs->data);
 		return 0;
 	}
-- 
2.33.0


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

* [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases
  2021-10-20 11:12 ` [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-20 11:12   ` Xueming Li
  2021-10-20 11:55     ` Gaëtan Rivet
  2 siblings, 1 reply; 43+ messages in thread
From: Xueming Li @ 2021-10-20 11:12 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

Initial version to test global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/meson.build    |   5 ++
 app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+)
 create mode 100644 app/test/test_devargs.c

diff --git a/app/test/meson.build b/app/test/meson.build
index a16374b7a10..c4b0241010d 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
     fast_tests += [['latencystats_autotest', true]]
     fast_tests += [['pdump_autotest', true]]
 endif
+if dpdk_conf.has('RTE_NET_VIRTIO')
+    test_deps += 'net_virtio'
+    test_sources += 'test_devargs.c'
+    fast_tests += [['devargs_autotest', true]]
+endif
 
 if dpdk_conf.has('RTE_LIB_POWER')
     test_deps += 'power'
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..13e95f052b0
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+#include <rte_bus.h>
+#include <rte_class.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct {
+		const char *devargs;
+		int bus_kv;
+		int class_kv;
+		int driver_kv;
+		const char *bus;
+		const char *name;
+		const char *class;
+	} list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci",
+		  1, 0, 0, "pci", NULL, NULL},
+		{ "class=eth",
+		  0, 1, 0, NULL, NULL, "eth" },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0",
+		  2, 1, 2, "pci", "0000:01:02.3", "eth" },
+		{ "bus=vdev,name=/dev/file/name/class=eth",
+		  2, 1, 0, "vdev", "/dev/file/name", "eth" },
+		{ "bus=vdev,name=/class/bus/path/class=eth",
+		  2, 1, 0, "vdev", "/class/bus/path", "eth" },
+		{ "bus=vdev,name=///dblslsh/class=eth",
+		  2, 1, 0, "vdev", "///dblslsh", "eth" },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0,
+		  "pci", "1:2.3", NULL },
+		{ "pci:1:2.3,k0=v0",
+		  0, 0, 1, "pci", "1:2.3", NULL },
+		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+		{ "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+	};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if ((list[i].bus_kv > 0 || list[i].bus != NULL) &&
+		    da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].bus != NULL &&
+		    strcmp(da.bus->name, list[i].bus) != 0) {
+			printf("rte_devargs_parse(%s) bus name (%s) not expected (%s)\n",
+			       list[i].devargs, da.bus->name, list[i].bus);
+			goto fail;
+		}
+		if ((list[i].class_kv > 0 || list[i].class != NULL) &&
+		    da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (list[i].class != NULL &&
+		    strcmp(da.cls->name, list[i].class) != 0) {
+			printf("rte_devargs_parse(%s) class name (%s) not expected (%s)\n",
+			       list[i].devargs, da.cls->name, list[i].class);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		if (list[i].name != NULL &&
+		    strcmp(da.name, list[i].name) != 0) {
+			printf("rte_devargs_parse(%s) device name (%s) not expected (%s)\n",
+			       list[i].devargs, da.name, list[i].name);
+			goto fail;
+		}
+		goto cleanup;
+fail:
+		fail = -1;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases
  2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-20 11:55     ` Gaëtan Rivet
  2021-10-20 13:53       ` Xueming(Steven) Li
  0 siblings, 1 reply; 43+ messages in thread
From: Gaëtan Rivet @ 2021-10-20 11:55 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev, David Marchand; +Cc: Thomas Monjalon, Lior Margalit

On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> Initial version to test global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  app/test/meson.build    |   5 ++
>  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+)
>  create mode 100644 app/test/test_devargs.c
>
> diff --git a/app/test/meson.build b/app/test/meson.build
> index a16374b7a10..c4b0241010d 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
>      fast_tests += [['latencystats_autotest', true]]
>      fast_tests += [['pdump_autotest', true]]
>  endif
> +if dpdk_conf.has('RTE_NET_VIRTIO')
> +    test_deps += 'net_virtio'
> +    test_sources += 'test_devargs.c'
> +    fast_tests += [['devargs_autotest', true]]
> +endif

Hi Steven,

Thanks for adding new use-cases and expanding the expect.
The dep check for the test can be improved I think.

When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.

Additionally, even if net_virtio is not built, a subset of your test remains valid.

Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.

Instead of checking the dependencies in the meson file, you could detect support in preamble
of the test:

bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
bool eth_class_avail = [...]

Then during the test case, depending on the expect part of list[i],
you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
message could be printed to say that the test was skipped.

-- 
Gaetan Rivet

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

* Re: [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases
  2021-10-20 11:55     ` Gaëtan Rivet
@ 2021-10-20 13:53       ` Xueming(Steven) Li
  2021-10-20 14:22         ` Gaëtan Rivet
  0 siblings, 1 reply; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-20 13:53 UTC (permalink / raw)
  To: dev, david.marchand, grive; +Cc: Lior Margalit, NBU-Contact-Thomas Monjalon

On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> > Initial version to test global devargs syntax.
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> >  app/test/meson.build    |   5 ++
> >  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 189 insertions(+)
> >  create mode 100644 app/test/test_devargs.c
> > 
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index a16374b7a10..c4b0241010d 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> >      fast_tests += [['latencystats_autotest', true]]
> >      fast_tests += [['pdump_autotest', true]]
> >  endif
> > +if dpdk_conf.has('RTE_NET_VIRTIO')
> > +    test_deps += 'net_virtio'
> > +    test_sources += 'test_devargs.c'
> > +    fast_tests += [['devargs_autotest', true]]
> > +endif
> 
> Hi Steven,
> 
> Thanks for adding new use-cases and expanding the expect.
> The dep check for the test can be improved I think.
> 
> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
> 
> Additionally, even if net_virtio is not built, a subset of your test remains valid.
> 
> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
> 
> Instead of checking the dependencies in the meson file, you could detect support in preamble
> of the test:
> 
> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
> bool eth_class_avail = [...]
> 
> Then during the test case, depending on the expect part of list[i],
> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
> message could be printed to say that the test was skipped.
> 

For vdev supported drivers, currently no API to find it. Macro worked
for me:

#ifdef RTE_NET_VIRTIO
		{ "net_virtio_user0,iface=test,path=/dev/vhost-
net,queues=1",
		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
		{
"net_virtio_user0,iface=test,path=/class/bus/,queues=1",
		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
#endif

PCI, vdev and eth is enabled by default, seems we don't need any flag.
But macros can be used as well to be safe, how do you think?


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

* Re: [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases
  2021-10-20 13:53       ` Xueming(Steven) Li
@ 2021-10-20 14:22         ` Gaëtan Rivet
  2021-10-20 14:34           ` Bruce Richardson
  0 siblings, 1 reply; 43+ messages in thread
From: Gaëtan Rivet @ 2021-10-20 14:22 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev, David Marchand; +Cc: Lior Margalit, Thomas Monjalon



On Wed, Oct 20, 2021, at 15:53, Xueming(Steven) Li wrote:
> On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
>> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
>> > Initial version to test global devargs syntax.
>> > 
>> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> > ---
>> >  app/test/meson.build    |   5 ++
>> >  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 189 insertions(+)
>> >  create mode 100644 app/test/test_devargs.c
>> > 
>> > diff --git a/app/test/meson.build b/app/test/meson.build
>> > index a16374b7a10..c4b0241010d 100644
>> > --- a/app/test/meson.build
>> > +++ b/app/test/meson.build
>> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
>> >      fast_tests += [['latencystats_autotest', true]]
>> >      fast_tests += [['pdump_autotest', true]]
>> >  endif
>> > +if dpdk_conf.has('RTE_NET_VIRTIO')
>> > +    test_deps += 'net_virtio'
>> > +    test_sources += 'test_devargs.c'
>> > +    fast_tests += [['devargs_autotest', true]]
>> > +endif
>> 
>> Hi Steven,
>> 
>> Thanks for adding new use-cases and expanding the expect.
>> The dep check for the test can be improved I think.
>> 
>> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
>> 
>> Additionally, even if net_virtio is not built, a subset of your test remains valid.
>> 
>> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
>> 
>> Instead of checking the dependencies in the meson file, you could detect support in preamble
>> of the test:
>> 
>> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
>> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
>> bool eth_class_avail = [...]
>> 
>> Then during the test case, depending on the expect part of list[i],
>> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
>> message could be printed to say that the test was skipped.
>> 
>
> For vdev supported drivers, currently no API to find it. Macro worked
> for me:
>
> #ifdef RTE_NET_VIRTIO
> 		{ "net_virtio_user0,iface=test,path=/dev/vhost-
> net,queues=1",
> 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> 		{
> "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> #endif
>
> PCI, vdev and eth is enabled by default, seems we don't need any flag.
> But macros can be used as well to be safe, how do you think?

With the macro you will go back to the same issue the meson-based dep check has:
the macro will be set, but it's possible the relevant .so won't be loaded when
executing the test.

For Busses and classes, similarly is there a chance that even though they are built,
they won't be loaded at runtime and make the test result incorrect?

What do you think about switching from net_virtio to net_ring instead otherwise?
It seems a lighter, utility driver that might be present even in minimal builds.
It is used in EAL tests, I think it makes more sense than net_virtio for unit tests.

The virtio-specific path-based parameters are irrelevant there, the actual
driver probe won't be executed anyway so any parameter looking like a path will
test properly.

Checkout app/test/test_eal_flags.c for examples of net_ring dummy devargs.

-- 
Gaetan Rivet

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

* Re: [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases
  2021-10-20 14:22         ` Gaëtan Rivet
@ 2021-10-20 14:34           ` Bruce Richardson
  0 siblings, 0 replies; 43+ messages in thread
From: Bruce Richardson @ 2021-10-20 14:34 UTC (permalink / raw)
  To: Gaëtan Rivet
  Cc: Xueming(Steven) Li, dev, David Marchand, Lior Margalit, Thomas Monjalon

On Wed, Oct 20, 2021 at 04:22:54PM +0200, Gaëtan Rivet wrote:
> 
> 
> On Wed, Oct 20, 2021, at 15:53, Xueming(Steven) Li wrote:
> > On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
> >> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> >> > Initial version to test global devargs syntax.
> >> > 
> >> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> >> > ---
> >> >  app/test/meson.build    |   5 ++
> >> >  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 189 insertions(+)
> >> >  create mode 100644 app/test/test_devargs.c
> >> > 
> >> > diff --git a/app/test/meson.build b/app/test/meson.build
> >> > index a16374b7a10..c4b0241010d 100644
> >> > --- a/app/test/meson.build
> >> > +++ b/app/test/meson.build
> >> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> >> >      fast_tests += [['latencystats_autotest', true]]
> >> >      fast_tests += [['pdump_autotest', true]]
> >> >  endif
> >> > +if dpdk_conf.has('RTE_NET_VIRTIO')
> >> > +    test_deps += 'net_virtio'
> >> > +    test_sources += 'test_devargs.c'
> >> > +    fast_tests += [['devargs_autotest', true]]
> >> > +endif
> >> 
> >> Hi Steven,
> >> 
> >> Thanks for adding new use-cases and expanding the expect.
> >> The dep check for the test can be improved I think.
> >> 
> >> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
> >> 
> >> Additionally, even if net_virtio is not built, a subset of your test remains valid.
> >> 
> >> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
> >> 
> >> Instead of checking the dependencies in the meson file, you could detect support in preamble
> >> of the test:
> >> 
> >> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
> >> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
> >> bool eth_class_avail = [...]
> >> 
> >> Then during the test case, depending on the expect part of list[i],
> >> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
> >> message could be printed to say that the test was skipped.
> >> 
> >
> > For vdev supported drivers, currently no API to find it. Macro worked
> > for me:
> >
> > #ifdef RTE_NET_VIRTIO
> > 		{ "net_virtio_user0,iface=test,path=/dev/vhost-
> > net,queues=1",
> > 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> > 		{
> > "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> > 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> > #endif
> >
> > PCI, vdev and eth is enabled by default, seems we don't need any flag.
> > But macros can be used as well to be safe, how do you think?
> 
> With the macro you will go back to the same issue the meson-based dep check has:
> the macro will be set, but it's possible the relevant .so won't be loaded when
> executing the test.
> 
> For Busses and classes, similarly is there a chance that even though they are built,
> they won't be loaded at runtime and make the test result incorrect?
> 
> What do you think about switching from net_virtio to net_ring instead otherwise?
> It seems a lighter, utility driver that might be present even in minimal builds.
> It is used in EAL tests, I think it makes more sense than net_virtio for unit tests.
> 
> The virtio-specific path-based parameters are irrelevant there, the actual
> driver probe won't be executed anyway so any parameter looking like a path will
> test properly.
> 
> Checkout app/test/test_eal_flags.c for examples of net_ring dummy devargs.
> 
I'd also recommend making use of the "TEST_SKIPPED" return value rather
than erroring out if a necessary dependency can't be found.

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

* [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax
  2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
                   ` (4 preceding siblings ...)
  2021-10-20 11:12 ` [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20 15:47 ` Xueming Li
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 1/3] devargs: support path value for global device arguments Xueming Li
                     ` (3 more replies)
  5 siblings, 4 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 15:47 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

- Support path in global syntax.
- Fix bus name resolving
- Add devargs test cases

v1: initial version
v2:
 - add test cases to test suite
 - add more test cases, verify device name, bus name and class name
v3:
 - remove autotest_data.py
v4:
 - make devargs test depends on virtio driver
v5:
 - use api to filter out unsupported devarg test cases

Email thread: 20211005123012.264727-1-xuemingl@nvidia.com

Xueming Li (3):
  devargs: support path value for global device arguments
  devargs: make bus key parsing optional
  test/devargs: add devargs test cases

 app/test/meson.build                |   2 +
 app/test/test_devargs.c             | 214 ++++++++++++++++++++++++++++
 drivers/bus/pci/pci_params.c        |   8 +-
 lib/eal/common/eal_common_devargs.c | 119 ++++++----------
 4 files changed, 263 insertions(+), 80 deletions(-)
 create mode 100644 app/test/test_devargs.c

-- 
2.33.0


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

* [dpdk-dev] [PATCH v5 1/3] devargs: support path value for global device arguments
  2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
@ 2021-10-20 15:47   ` Xueming Li
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 2/3] devargs: make bus key parsing optional Xueming Li
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 15:47 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

Slash is used to split global device arguments.

To support path value which contains slash, this patch parses devargs by
locating both slash and layer name key:
  bus=a,name=/some/path/class=b,k1=v1/driver=c,k2=v2
"/class=" and "/driver" are valid start of a layer.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 lib/eal/common/eal_common_devargs.c | 117 ++++++++++------------------
 1 file changed, 43 insertions(+), 74 deletions(-)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 411dd6a75f6..d673598032d 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -29,18 +29,6 @@ TAILQ_HEAD(rte_devargs_list, rte_devargs);
 static struct rte_devargs_list devargs_list =
 	TAILQ_HEAD_INITIALIZER(devargs_list);
 
-static size_t
-devargs_layer_count(const char *s)
-{
-	size_t i = s ? 1 : 0;
-
-	while (s != NULL && s[0] != '\0') {
-		i += s[0] == '/';
-		s++;
-	}
-	return i;
-}
-
 /* Resolve devargs name from bus arguments. */
 static int
 devargs_bus_parse_default(struct rte_devargs *devargs,
@@ -77,23 +65,13 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
 	};
 	struct rte_kvargs_pair *kv = NULL;
-	struct rte_class *cls = NULL;
-	struct rte_bus *bus = NULL;
-	const char *s = devstr;
-	size_t nblayer;
-	size_t i = 0;
+	struct rte_kvargs *bus_kvlist = NULL;
+	char *s;
+	size_t nblayer = 0;
+	size_t i;
 	int ret = 0;
 	bool allocated_data = false;
 
-	/* Split each sub-lists. */
-	nblayer = devargs_layer_count(devstr);
-	if (nblayer > RTE_DIM(layers)) {
-		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
-			nblayer);
-		ret = -E2BIG;
-		goto get_out;
-	}
-
 	/* If the devargs points the devstr
 	 * as source data, then it should not allocate
 	 * anything and keep referring only to it.
@@ -106,33 +84,41 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 			goto get_out;
 		}
 		allocated_data = true;
-		s = devargs->data;
 	}
+	s = devargs->data;
 
 	while (s != NULL) {
-		if (i >= RTE_DIM(layers)) {
-			RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
-			ret = -EINVAL;
+		if (nblayer > RTE_DIM(layers)) {
+			ret = -E2BIG;
 			goto get_out;
 		}
-		/*
-		 * The last layer is free-form.
-		 * The "driver" key is not required (but accepted).
-		 */
-		if (strncmp(layers[i].key, s, strlen(layers[i].key)) &&
-				i != RTE_DIM(layers) - 1)
-			goto next_layer;
-		layers[i].str = s;
-		layers[i].kvlist = rte_kvargs_parse_delim(s, NULL, "/");
-		if (layers[i].kvlist == NULL) {
+		layers[nblayer].str = s;
+
+		/* Locate next layer starts with valid layer key. */
+		while (s != NULL) {
+			s = strchr(s, '/');
+			if (s == NULL)
+				break;
+			for (i = 0; i < RTE_DIM(layers); i++) {
+				if (strncmp(s + 1, layers[i].key,
+					    strlen(layers[i].key)) == 0) {
+					*s = '\0';
+					break;
+				}
+			}
+			s++;
+			if (i < RTE_DIM(layers))
+				break;
+		}
+
+		layers[nblayer].kvlist = rte_kvargs_parse
+				(layers[nblayer].str, NULL);
+		if (layers[nblayer].kvlist == NULL) {
 			ret = -EINVAL;
 			goto get_out;
 		}
-		s = strchr(s, '/');
-		if (s != NULL)
-			s++;
-next_layer:
-		i++;
+
+		nblayer++;
 	}
 
 	/* Parse each sub-list. */
@@ -143,52 +129,35 @@ rte_devargs_layers_parse(struct rte_devargs *devargs,
 		if (kv->key == NULL)
 			continue;
 		if (strcmp(kv->key, RTE_DEVARGS_KEY_BUS) == 0) {
-			bus = rte_bus_find_by_name(kv->value);
-			if (bus == NULL) {
+			bus_kvlist = layers[i].kvlist;
+			devargs->bus_str = layers[i].str;
+			devargs->bus = rte_bus_find_by_name(kv->value);
+			if (devargs->bus == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_CLASS) == 0) {
-			cls = rte_class_find_by_name(kv->value);
-			if (cls == NULL) {
+			devargs->cls_str = layers[i].str;
+			devargs->cls = rte_class_find_by_name(kv->value);
+			if (devargs->cls == NULL) {
 				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
 					kv->value);
 				ret = -EFAULT;
 				goto get_out;
 			}
 		} else if (strcmp(kv->key, RTE_DEVARGS_KEY_DRIVER) == 0) {
-			/* Ignore */
+			devargs->drv_str = layers[i].str;
 			continue;
 		}
 	}
 
-	/* Fill devargs fields. */
-	devargs->bus_str = layers[0].str;
-	devargs->cls_str = layers[1].str;
-	devargs->drv_str = layers[2].str;
-	devargs->bus = bus;
-	devargs->cls = cls;
-
-	/* If we own the data, clean up a bit
-	 * the several layers string, to ease
-	 * their parsing afterward.
-	 */
-	if (devargs->data != devstr) {
-		char *s = devargs->data;
-
-		while ((s = strchr(s, '/'))) {
-			*s = '\0';
-			s++;
-		}
-	}
-
 	/* Resolve devargs name. */
-	if (bus != NULL && bus->devargs_parse != NULL)
-		ret = bus->devargs_parse(devargs);
-	else if (layers[0].kvlist != NULL)
-		ret = devargs_bus_parse_default(devargs, layers[0].kvlist);
+	if (devargs->bus != NULL && devargs->bus->devargs_parse != NULL)
+		ret = devargs->bus->devargs_parse(devargs);
+	else if (bus_kvlist != NULL)
+		ret = devargs_bus_parse_default(devargs, bus_kvlist);
 
 get_out:
 	for (i = 0; i < RTE_DIM(layers); i++) {
-- 
2.33.0


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

* [dpdk-dev] [PATCH v5 2/3] devargs: make bus key parsing optional
  2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 1/3] devargs: support path value for global device arguments Xueming Li
@ 2021-10-20 15:47   ` Xueming Li
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases Xueming Li
  2021-10-21  9:22   ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax David Marchand
  3 siblings, 0 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 15:47 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

Global devargs syntax is used as device iteration filter like
"class=vdpa", a devargs without bus args is valid from parsing
perspective.

This patch makes bus args optional.

Fixes: d2a66ad79480 ("bus: add device arguments name parsing")

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Reviewed-by: Gaetan Rivet <grive@u256.net>
---
 drivers/bus/pci/pci_params.c        | 8 +++-----
 lib/eal/common/eal_common_devargs.c | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/pci_params.c b/drivers/bus/pci/pci_params.c
index 21c2e1d0368..60b424b8297 100644
--- a/drivers/bus/pci/pci_params.c
+++ b/drivers/bus/pci/pci_params.c
@@ -87,11 +87,10 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 	struct rte_kvargs *kvargs;
 	const char *addr_str;
 	struct rte_pci_addr addr;
-	int ret;
+	int ret = 0;
 
-	if (da == NULL)
+	if (da == NULL || da->bus_str == NULL)
 		return 0;
-	RTE_ASSERT(da->bus_str != NULL);
 
 	kvargs = rte_kvargs_parse(da->bus_str, NULL);
 	if (kvargs == NULL) {
@@ -103,9 +102,8 @@ rte_pci_devargs_parse(struct rte_devargs *da)
 
 	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
 	if (addr_str == NULL) {
-		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
+		RTE_LOG(DEBUG, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
 			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
-		ret = -ENODEV;
 		goto out;
 	}
 
diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index d673598032d..8c7650cf6c2 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -39,7 +39,7 @@ devargs_bus_parse_default(struct rte_devargs *devargs,
 	/* Parse devargs name from bus key-value list. */
 	name = rte_kvargs_get(bus_args, "name");
 	if (name == NULL) {
-		RTE_LOG(INFO, EAL, "devargs name not found: %s\n",
+		RTE_LOG(DEBUG, EAL, "devargs name not found: %s\n",
 			devargs->data);
 		return 0;
 	}
-- 
2.33.0


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

* [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases
  2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 1/3] devargs: support path value for global device arguments Xueming Li
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 2/3] devargs: make bus key parsing optional Xueming Li
@ 2021-10-20 15:47   ` Xueming Li
  2021-10-21  9:03     ` Gaëtan Rivet
  2021-10-23  6:17     ` David Marchand
  2021-10-21  9:22   ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax David Marchand
  3 siblings, 2 replies; 43+ messages in thread
From: Xueming Li @ 2021-10-20 15:47 UTC (permalink / raw)
  To: dev, Gaetan Rivet, David Marchand
  Cc: xuemingl, Thomas Monjalon, Lior Margalit

Initial version to test global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/meson.build    |   2 +
 app/test/test_devargs.c | 214 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 216 insertions(+)
 create mode 100644 app/test/test_devargs.c

diff --git a/app/test/meson.build b/app/test/meson.build
index a16374b7a10..16f4ff14ee8 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -42,6 +42,7 @@ test_sources = files(
         'test_cryptodev_security_pdcp.c',
         'test_cycles.c',
         'test_debug.c',
+		'test_devargs.c',
         'test_distributor.c',
         'test_distributor_perf.c',
         'test_dmadev.c',
@@ -204,6 +205,7 @@ fast_tests = [
         ['common_autotest', true],
         ['cpuflags_autotest', true],
         ['debug_autotest', true],
+		['devargs_autotest', true],
         ['eal_flags_c_opt_autotest', false],
         ['eal_flags_main_opt_autotest', false],
         ['eal_flags_n_opt_autotest', false],
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..19036716bf7
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,214 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+#include <rte_bus.h>
+#include <rte_class.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+struct devargs_case {
+	const char *devargs;
+	int bus_kv;
+	int class_kv;
+	int driver_kv;
+	const char *bus;
+	const char *name;
+	const char *class;
+};
+
+static int
+test_valid_devargs_cases(const struct devargs_case *list, size_t n)
+{
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = TEST_SUCCESS;
+	struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+	struct rte_class *eth_class = rte_class_find_by_name("eth");
+
+	for (i = 0; i < n; i++) {
+		if (pci_bus == NULL && list[i].bus != NULL &&
+		    strcmp(list[i].bus, "pci") == 0)
+			continue;
+		if (vdev_bus == NULL && list[i].bus != NULL &&
+		    strcmp(list[i].bus, "vdev") == 0)
+			continue;
+		if (eth_class == NULL && list[i].class != NULL &&
+		    strcmp(list[i].class, "eth") == 0)
+			continue;
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if ((list[i].bus_kv > 0 || list[i].bus != NULL) &&
+		    da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].bus != NULL &&
+		    strcmp(da.bus->name, list[i].bus) != 0) {
+			printf("rte_devargs_parse(%s) bus name (%s) not expected (%s)\n",
+			       list[i].devargs, da.bus->name, list[i].bus);
+			goto fail;
+		}
+		if ((list[i].class_kv > 0 || list[i].class != NULL) &&
+		    da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (list[i].class != NULL &&
+		    strcmp(da.cls->name, list[i].class) != 0) {
+			printf("rte_devargs_parse(%s) class name (%s) not expected (%s)\n",
+			       list[i].devargs, da.cls->name, list[i].class);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		if (list[i].name != NULL &&
+		    strcmp(da.name, list[i].name) != 0) {
+			printf("rte_devargs_parse(%s) device name (%s) not expected (%s)\n",
+			       list[i].devargs, da.name, list[i].name);
+			goto fail;
+		}
+		goto cleanup;
+fail:
+		fail = TEST_FAILED;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct devargs_case list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci",
+		  1, 0, 0, "pci", NULL, NULL},
+		{ "class=eth",
+		  0, 1, 0, NULL, NULL, "eth" },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0",
+		  2, 1, 2, "pci", "0000:01:02.3", "eth" },
+		{ "bus=vdev,name=/dev/file/name/class=eth",
+		  2, 1, 0, "vdev", "/dev/file/name", "eth" },
+		{ "bus=vdev,name=/class/bus/path/class=eth",
+		  2, 1, 0, "vdev", "/class/bus/path", "eth" },
+		{ "bus=vdev,name=///dblslsh/class=eth",
+		  2, 1, 0, "vdev", "///dblslsh", "eth" },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0,
+		  "pci", "1:2.3", NULL },
+		{ "pci:1:2.3,k0=v0",
+		  0, 0, 1, "pci", "1:2.3", NULL },
+	};
+	static const struct devargs_case legacy_ring_list[] = {
+		{ "net_ring0",
+		  0, 0, 0, "vdev", "net_ring0", NULL },
+		{ "net_ring0,iface=test,path=/class/bus/,queues=1",
+		  0, 0, 3, "vdev", "net_ring0", NULL },
+	};
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+	int ret;
+
+	ret = test_valid_devargs_cases(list, RTE_DIM(list));
+	if (vdev_bus != NULL && vdev_bus->parse("net_ring0", NULL) == 0)
+		/* Ring vdev driver enabled. */
+		ret |= test_valid_devargs_cases(legacy_ring_list,
+						RTE_DIM(legacy_ring_list));
+	return ret;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-21  9:03     ` Gaëtan Rivet
  2021-10-23  6:17     ` David Marchand
  1 sibling, 0 replies; 43+ messages in thread
From: Gaëtan Rivet @ 2021-10-21  9:03 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev, David Marchand; +Cc: Thomas Monjalon, Lior Margalit

On Wed, Oct 20, 2021, at 17:47, Xueming Li wrote:
> Initial version to test global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>

Hi,

Thanks for making the changes.
I have the nagging impression that the runtime checks could have been written
in a lighter way, but maybe that's just it. It seems to fit the function, so
it will do.

Thanks again for dealing with this, and it's very nice to have those unit tests.

Reviewed-by: Gaetan Rivet <grive@u256.net>

-- 
Gaetan Rivet

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

* Re: [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax
  2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
                     ` (2 preceding siblings ...)
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases Xueming Li
@ 2021-10-21  9:22   ` David Marchand
  2021-10-21 10:43     ` Xueming(Steven) Li
  3 siblings, 1 reply; 43+ messages in thread
From: David Marchand @ 2021-10-21  9:22 UTC (permalink / raw)
  To: Xueming Li; +Cc: dev, Gaetan Rivet, Thomas Monjalon, Lior Margalit

On Wed, Oct 20, 2021 at 5:47 PM Xueming Li <xuemingl@nvidia.com> wrote:
>
> - Support path in global syntax.
> - Fix bus name resolving
> - Add devargs test cases
>
> v1: initial version
> v2:
>  - add test cases to test suite
>  - add more test cases, verify device name, bus name and class name
> v3:
>  - remove autotest_data.py
> v4:
>  - make devargs test depends on virtio driver
> v5:
>  - use api to filter out unsupported devarg test cases

Fixed style issues in meson (this is new in v5, please pay attention
to such details) and applied.
Thanks.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax
  2021-10-21  9:22   ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax David Marchand
@ 2021-10-21 10:43     ` Xueming(Steven) Li
  0 siblings, 0 replies; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-21 10:43 UTC (permalink / raw)
  To: david.marchand; +Cc: Lior Margalit, NBU-Contact-Thomas Monjalon, dev, grive

On Thu, 2021-10-21 at 11:22 +0200, David Marchand wrote:
> On Wed, Oct 20, 2021 at 5:47 PM Xueming Li <xuemingl@nvidia.com> wrote:
> > 
> > - Support path in global syntax.
> > - Fix bus name resolving
> > - Add devargs test cases
> > 
> > v1: initial version
> > v2:
> >  - add test cases to test suite
> >  - add more test cases, verify device name, bus name and class name
> > v3:
> >  - remove autotest_data.py
> > v4:
> >  - make devargs test depends on virtio driver
> > v5:
> >  - use api to filter out unsupported devarg test cases
> 
> Fixed style issues in meson (this is new in v5, please pay attention
> to such details) and applied.
> Thanks.
> 

Thanks David!

> 


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

* Re: [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases
  2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases Xueming Li
  2021-10-21  9:03     ` Gaëtan Rivet
@ 2021-10-23  6:17     ` David Marchand
  2021-10-23 12:20       ` Xueming(Steven) Li
  1 sibling, 1 reply; 43+ messages in thread
From: David Marchand @ 2021-10-23  6:17 UTC (permalink / raw)
  To: Xueming Li; +Cc: dev, Gaetan Rivet, Thomas Monjalon, Lior Margalit

On Wed, Oct 20, 2021 at 5:48 PM Xueming Li <xuemingl@nvidia.com> wrote:
> +       kvlist = rte_kvargs_parse(args, NULL);
> +       if (kvlist == NULL) {
> +               printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
> +                      devargs, layer, args);
> +               return -1;
> +       }
> +       if ((int)kvlist->count != n) {
> +               printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
> +                      devargs, layer, args, kvlist->count, n);
> +               return -1;
> +       }
> +       return 0;
> +}

Missing some rte_kvargs_free().
Can you send a fix?

Thanks.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases
  2021-10-23  6:17     ` David Marchand
@ 2021-10-23 12:20       ` Xueming(Steven) Li
  0 siblings, 0 replies; 43+ messages in thread
From: Xueming(Steven) Li @ 2021-10-23 12:20 UTC (permalink / raw)
  To: david.marchand; +Cc: Lior Margalit, NBU-Contact-Thomas Monjalon, dev, grive

On Sat, 2021-10-23 at 08:17 +0200, David Marchand wrote:
> On Wed, Oct 20, 2021 at 5:48 PM Xueming Li <xuemingl@nvidia.com> wrote:
> > +       kvlist = rte_kvargs_parse(args, NULL);
> > +       if (kvlist == NULL) {
> > +               printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
> > +                      devargs, layer, args);
> > +               return -1;
> > +       }
> > +       if ((int)kvlist->count != n) {
> > +               printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
> > +                      devargs, layer, args, kvlist->count, n);
> > +               return -1;
> > +       }
> > +       return 0;
> > +}
> 
> Missing some rte_kvargs_free().
> Can you send a fix?

Thanks for checking this out, fix posted:
https://patches.dpdk.org/project/dpdk/patch/20211023121755.169290-1-xuemingl@nvidia.com/


> 
> Thanks.
> 
> 


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

end of thread, other threads:[~2021-10-23 12:20 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05 12:30 [dpdk-dev] [PATCH 1/3] devargs: support path value for global device arguments Xueming Li
2021-10-05 12:30 ` [dpdk-dev] [PATCH 2/3] devargs: make bus key parsing optional Xueming Li
2021-10-05 12:30 ` [dpdk-dev] [PATCH 3/3] test/devargs: add devargs test cases Xueming Li
2021-10-05 14:01   ` David Marchand
2021-10-05 15:56     ` Xueming(Steven) Li
2021-10-05 15:54 ` [dpdk-dev] [PATCH v1 0/3] devargs: support path in global syntax Xueming Li
2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 1/3] devargs: support path value for global device arguments Xueming Li
2021-10-19 14:57     ` Gaëtan Rivet
2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 2/3] devargs: make bus key parsing optional Xueming Li
2021-10-19 15:19     ` Gaëtan Rivet
2021-10-05 15:54   ` [dpdk-dev] [PATCH v1 3/3] test/devargs: add devargs test cases Xueming Li
2021-10-19 15:07     ` Gaëtan Rivet
2021-10-20  7:20       ` Xueming(Steven) Li
2021-10-20  7:31   ` [dpdk-dev] [PATCH v2 0/3] devargs: support path in global syntax Xueming Li
2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 1/3] devargs: support path value for global device arguments Xueming Li
2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 2/3] devargs: make bus key parsing optional Xueming Li
2021-10-20  7:31     ` [dpdk-dev] [PATCH v2 3/3] test/devargs: add devargs test cases Xueming Li
2021-10-20  7:38       ` David Marchand
2021-10-20  8:23         ` Xueming(Steven) Li
2021-10-20  8:21 ` [dpdk-dev] [PATCH v3 0/3] devargs: support path in global syntax Xueming Li
2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 1/3] devargs: support path value for global device arguments Xueming Li
2021-10-20  8:21   ` [dpdk-dev] [PATCH v3 2/3] devargs: make bus key parsing optional Xueming Li
2021-10-20  8:22   ` [dpdk-dev] [PATCH v3 3/3] test/devargs: add devargs test cases Xueming Li
2021-10-20  9:08     ` David Marchand
2021-10-20  9:40       ` Xueming(Steven) Li
2021-10-20 10:41         ` Bruce Richardson
2021-10-20 11:12 ` [dpdk-dev] [PATCH v4 0/3] devargs: support path in global syntax Xueming Li
2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 1/3] devargs: support path value for global device arguments Xueming Li
2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 2/3] devargs: make bus key parsing optional Xueming Li
2021-10-20 11:12   ` [dpdk-dev] [PATCH v4 3/3] test/devargs: add devargs test cases Xueming Li
2021-10-20 11:55     ` Gaëtan Rivet
2021-10-20 13:53       ` Xueming(Steven) Li
2021-10-20 14:22         ` Gaëtan Rivet
2021-10-20 14:34           ` Bruce Richardson
2021-10-20 15:47 ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax Xueming Li
2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 1/3] devargs: support path value for global device arguments Xueming Li
2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 2/3] devargs: make bus key parsing optional Xueming Li
2021-10-20 15:47   ` [dpdk-dev] [PATCH v5 3/3] test/devargs: add devargs test cases Xueming Li
2021-10-21  9:03     ` Gaëtan Rivet
2021-10-23  6:17     ` David Marchand
2021-10-23 12:20       ` Xueming(Steven) Li
2021-10-21  9:22   ` [dpdk-dev] [PATCH v5 0/3] devargs: support path in global syntax David Marchand
2021-10-21 10:43     ` Xueming(Steven) Li

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