On Fri, Mar 23, 2018 at 07:45:03PM +0100, Gaetan Rivet wrote:
> Signed-off-by: Gaetan Rivet <gaetan.ri...@6wind.com>
> ---
>
> Cc: Neil Horman <nhor...@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
>
>
