[dpdk-dev] [PATCH 2/2] dev: use rte_kvargs

Neil Horman nhorman at tuxdriver.com
Mon Mar 26 13:38:19 CEST 2018


On Fri, Mar 23, 2018 at 07:45:03PM +0100, Gaetan Rivet wrote:
> Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> ---
> 
> Cc: Neil Horman <nhorman at tuxdriver.com>
> 
I'm actually ok with this, but as Keith noted, I'm not sure why you didn't just:

1) Add the ability to create a grouping key, so that key value pairs could
contain a list of comma separated values (something like '{}' to denote that
everything between the characters was the value in a kv pair, regardless of
other tokenizing characters in the value).

2) Add the ability to recursively parse the value into a list of tokens

3) Layer your functionality on top of (1) and (2), as Keith noted

Neil

> I find using rte_parse_kv cleaner.
> The function rte_dev_iterator_init is already ugly enough as it is.
> This is really not helping.
> 
>  lib/librte_eal/common/eal_common_dev.c | 127 +++++++++++++++++++++------------
>  lib/librte_eal/linuxapp/eal/Makefile   |   1 +
>  2 files changed, 83 insertions(+), 45 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index 21703b777..9f1a0ebda 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -15,6 +15,7 @@
>  #include <rte_devargs.h>
>  #include <rte_debug.h>
>  #include <rte_errno.h>
> +#include <rte_kvargs.h>
>  #include <rte_log.h>
>  
>  #include "eal_private.h"
> @@ -270,12 +271,15 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
>  }
>  
>  int __rte_experimental
> -rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str)
> +rte_dev_iterator_init(struct rte_dev_iterator *it,
> +		      const char *devstr)
>  {
> -	struct rte_bus *bus = NULL;
> +	struct rte_kvargs *kvlist = NULL;
>  	struct rte_class *cls = NULL;
> -	struct rte_kvarg kv;
> -	char *slash;
> +	struct rte_bus *bus = NULL;
> +	struct rte_kvargs_pair *kv;
> +	char *slash = NULL;
> +	char *str = NULL;
>  
>  	/* Having both busstr and clsstr NULL is illegal,
>  	 * marking this iterator as invalid unless
> @@ -283,98 +287,131 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str)
>  	 */
>  	it->busstr = NULL;
>  	it->clsstr = NULL;
> +	str = strdup(devstr);
> +	if (str == NULL) {
> +		rte_errno = ENOMEM;
> +		goto get_out;
> +	}
> +	slash = strchr(str, '/');
> +	if (slash != NULL) {
> +		slash[0] = '\0';
> +		slash = strchr(devstr, '/') + 1;
> +	}
>  	/* Safety checks and prep-work */
> -	if (rte_parse_kv(str, &kv)) {
> +	kvlist = rte_kvargs_parse(str, NULL);
> +	if (kvlist == NULL) {
>  		RTE_LOG(ERR, EAL, "Could not parse: %s\n", str);
>  		rte_errno = EINVAL;
> -		return -rte_errno;
> +		goto get_out;
>  	}
>  	it->device = NULL;
>  	it->class_device = NULL;
> -	if (strcmp(kv.key, "bus") == 0) {
> -		bus = rte_bus_find_by_name(kv.value);
> +	kv = &kvlist->pairs[0];
> +	if (strcmp(kv->key, "bus") == 0) {
> +		bus = rte_bus_find_by_name(kv->value);
>  		if (bus == NULL) {
>  			RTE_LOG(ERR, EAL, "Could not find bus \"%s\"\n",
> -				kv.value);
> +				kv->value);
>  			rte_errno = EFAULT;
> -			return -rte_errno;
> +			goto get_out;
>  		}
> -		slash = strchr(str, '/');
>  		if (slash != NULL) {
> -			if (rte_parse_kv(slash + 1, &kv)) {
> +			rte_kvargs_free(kvlist);
> +			kvlist = rte_kvargs_parse(slash, NULL);
> +			if (kvlist == NULL) {
>  				RTE_LOG(ERR, EAL, "Could not parse: %s\n",
> -					slash + 1);
> +					slash);
>  				rte_errno = EINVAL;
> -				return -rte_errno;
> +				goto get_out;
>  			}
> -			cls = rte_class_find_by_name(kv.value);
> +			kv = &kvlist->pairs[0];
> +			if (strcmp(kv->key, "class")) {
> +				RTE_LOG(ERR, EAL, "Additional layer must be a class\n");
> +				rte_errno = EINVAL;
> +				goto get_out;
> +			}
> +			cls = rte_class_find_by_name(kv->value);
>  			if (cls == NULL) {
>  				RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
> -					kv.value);
> +					kv->value);
>  				rte_errno = EFAULT;
> -				return -rte_errno;
> +				goto get_out;
>  			}
>  		}
> -	} else if (strcmp(kv.key, "class") == 0) {
> -		cls = rte_class_find_by_name(kv.value);
> +	} else if (strcmp(kv->key, "class") == 0) {
> +		cls = rte_class_find_by_name(kv->value);
>  		if (cls == NULL) {
>  			RTE_LOG(ERR, EAL, "Could not find class \"%s\"\n",
> -				kv.value);
> +				kv->value);
>  			rte_errno = EFAULT;
> -			return -rte_errno;
> +			goto get_out;
>  		}
>  	} else {
>  		rte_errno = EINVAL;
> -		return -rte_errno;
> +		goto get_out;
>  	}
>  	/* The string should have at least
>  	 * one layer specified.
>  	 */
>  	if (bus == NULL && cls == NULL) {
>  		rte_errno = EINVAL;
> -		return -rte_errno;
> +		goto get_out;
>  	}
>  	if ((bus != NULL && bus->dev_iterate == NULL) ||
>  	    (cls != NULL && cls->dev_iterate == NULL)) {
>  		rte_errno = ENOTSUP;
> -		return -rte_errno;
> +		goto get_out;
>  	}
>  	if (bus != NULL) {
> -		it->busstr = str;
> +		it->busstr = devstr;
>  		if (cls != NULL)
> -			it->clsstr = slash + 1;
> +			it->clsstr = slash;
>  	} else if (cls != NULL) {
> -		it->clsstr = str;
> +		it->clsstr = devstr;
>  	}
> -	it->devstr = str;
> +	it->devstr = devstr;
>  	it->bus = bus;
>  	it->cls = cls;
> -	return 0;
> +get_out:
> +	rte_kvargs_free(kvlist);
> +	free(str);
> +	return -rte_errno;
> +}
> +
> +/* '\0' forbidden in sym */
> +static const char *
> +strfirstof(const char *str,
> +	   const char *sym)
> +{
> +	const char *s;
> +
> +	for (s = str; s[0] != '\0'; s++) {
> +		const char *c;
> +
> +		for (c = sym; c[0] != '\0'; c++) {
> +			if (c[0] == s[0])
> +				return s;
> +		}
> +	}
> +	return NULL;
>  }
>  
>  static char *
>  dev_str_sane_cpy(const char *str)
>  {
> -	struct rte_kvarg kv;
> -	char *end;
> +	const char *end;
>  	char *cpy;
>  
> -	if (rte_parse_kv(str, &kv)) {
> -		rte_errno = EINVAL;
> -		return NULL;
> -	}
> -	/* copying '\0' is valid. */
> -	if (kv.next != NULL)
> -		cpy = strdup(kv.next);
> -	else
> +	end = strfirstof(str, ",/");
> +	if (end != NULL &&
> +	    end[0] == ',') {
> +		cpy = strdup(end + 1);
> +	} else {
> +		/* '/' or '\0' */
>  		cpy = strdup("");
> -	if (cpy == NULL) {
> -		rte_errno = ENOMEM;
> -		return NULL;
>  	}
> -	end = strchr(cpy, '/');
> -	if (end != NULL)
> -		end[0] = '\0';
> +	if (cpy == NULL)
> +		rte_errno = ENOMEM;
>  	return cpy;
>  }
>  
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
> index a3edbbe76..87caa23a1 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -27,6 +27,7 @@ LDLIBS += -lrt
>  ifeq ($(CONFIG_RTE_EAL_NUMA_AWARE_HUGEPAGES),y)
>  LDLIBS += -lnuma
>  endif
> +LDLIBS += -lrte_kvargs
>  
>  # specific to linuxapp exec-env
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) := eal.c
> -- 
> 2.11.0
> 
> 


More information about the dev mailing list