diff mbox

[dpdk-dev,v2,08/15] devargs: use existing functions in rte_eal_devargs_parse()

Message ID 20170714211213.34436-9-jblunck@infradead.org (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Jan Blunck July 14, 2017, 9:12 p.m. UTC 
  This fixes the newly introduces rte_eal_devargs_parse() to make use of:
- snprintf() instead of open coding a while() loop
- rte_eal_parse_devargs_str() instead of duplicating parsing code
- RTE_LOG() instead of direct output to stderr

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_eal/common/eal_common_devargs.c | 57 +++++++++++++++---------------
 1 file changed, 28 insertions(+), 29 deletions(-)

Comments

Ferruh Yigit Sept. 4, 2017, 4:24 p.m. UTC | #1 
On 7/14/2017 10:12 PM, Jan Blunck wrote:
> This fixes the newly introduces rte_eal_devargs_parse() to make use of:
> - snprintf() instead of open coding a while() loop
> - rte_eal_parse_devargs_str() instead of duplicating parsing code
> - RTE_LOG() instead of direct output to stderr
> 
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
>  lib/librte_eal/common/eal_common_devargs.c | 57 +++++++++++++++---------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 205fabb95..b5273287e 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -87,54 +87,53 @@ int
>  rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)

here "const char *dev" is devarg_str, it can be something like:
"00:01.0,somekey=somevalue,keymore=valuemore"

Calling this "dev" is confusing I think, since you are touching to the
function does it make sense to rename this?

>  {
>  	struct rte_bus *bus = NULL;
> -	const char *devname;
> -	const size_t maxlen = sizeof(da->name);
> -	size_t i;
> +	char *devname = NULL, *drvargs = NULL;
> +	int ret;
>  
>  	if (dev == NULL || da == NULL)
>  		return -EINVAL;
>  	/* Retrieve eventual bus info */
>  	do {
> -		devname = dev;
>  		bus = rte_bus_find(bus, bus_name_cmp, dev);
>  		if (bus == NULL)
>  			break;
> -		devname = dev + strlen(bus->name) + 1;
> -		if (rte_bus_find_by_device_name(devname) == bus)
> +		dev += strlen(bus->name) + 1;

This deserves a comment I believe, what is done here?

> +		if (rte_bus_find_by_device_name(dev) == bus)
>  			break;
>  	} while (1);

<...>
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 205fabb95..b5273287e 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -87,54 +87,53 @@  int
 rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
 {
 	struct rte_bus *bus = NULL;
-	const char *devname;
-	const size_t maxlen = sizeof(da->name);
-	size_t i;
+	char *devname = NULL, *drvargs = NULL;
+	int ret;
 
 	if (dev == NULL || da == NULL)
 		return -EINVAL;
 	/* Retrieve eventual bus info */
 	do {
-		devname = dev;
 		bus = rte_bus_find(bus, bus_name_cmp, dev);
 		if (bus == NULL)
 			break;
-		devname = dev + strlen(bus->name) + 1;
-		if (rte_bus_find_by_device_name(devname) == bus)
+		dev += strlen(bus->name) + 1;
+		if (rte_bus_find_by_device_name(dev) == bus)
 			break;
 	} while (1);
+
+	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
+	if (ret != 0)
+		return ret;
+
 	/* Store device name */
-	i = 0;
-	while (devname[i] != '\0' && devname[i] != ',') {
-		da->name[i] = devname[i];
-		i++;
-		if (i == maxlen) {
-			fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
-				dev, maxlen);
-			da->name[i - 1] = '\0';
-			return -EINVAL;
-		}
+	ret = snprintf(da->name, sizeof(da->name), "%s", devname);
+	if (ret < 0 || ret >= (int)sizeof(da->name)) {
+		RTE_LOG(ERR, EAL, "Invalid device name: \"%s\"\n", devname);
+		ret = -EINVAL;
+		goto fail;
 	}
-	da->name[i] = '\0';
+
+	/* Store drivers arguments */
+	da->args = drvargs;
+	drvargs = NULL;
+
 	if (bus == NULL) {
 		bus = rte_bus_find_by_device_name(da->name);
 		if (bus == NULL) {
-			fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
+			RTE_LOG(ERR, EAL, "Failed to parse device: \"%s\"\n",
 				da->name);
-			return -EFAULT;
+			ret = -EFAULT;
+			goto fail;
 		}
 	}
 	da->bus = bus;
-	/* Parse eventual device arguments */
-	if (devname[i] == ',')
-		da->args = strdup(&devname[i + 1]);
-	else
-		da->args = strdup("");
-	if (da->args == NULL) {
-		fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
-		return -ENOMEM;
-	}
-	return 0;
+	ret = 0;
+
+fail:
+	free(devname);
+	free(drvargs);
+	return ret;
 }
 
 static const struct rte_bus_conf BUS_CONF_WHITELIST = {