From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id EE46B5F22 for ; Mon, 26 Mar 2018 13:39:00 +0200 (CEST) Received: from cpe-2606-a000-111b-40b7-640c-26a-4e16-9225.dyn6.twc.com ([2606:a000:111b:40b7:640c:26a:4e16:9225] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1f0QSh-00020u-Gk; Mon, 26 Mar 2018 07:38:59 -0400 Date: Mon, 26 Mar 2018 07:38:19 -0400 From: Neil Horman To: Gaetan Rivet Cc: dev@dpdk.org Message-ID: <20180326113819.GB14835@hmswarspite.think-freely.org> References: <20180323131236.yagasxv2nxorfjl5@bidouze.vm.6wind.com> <20180323184503.13041-1-gaetan.rivet@6wind.com> <20180323184503.13041-2-gaetan.rivet@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180323184503.13041-2-gaetan.rivet@6wind.com> User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCH 2/2] dev: use rte_kvargs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Mar 2018 11:39:01 -0000 On Fri, Mar 23, 2018 at 07:45:03PM +0100, Gaetan Rivet wrote: > Signed-off-by: Gaetan Rivet > --- > > Cc: Neil Horman > 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 > #include > #include > +#include > #include > > #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 > >