[dpdk-dev,2/2] dev: use rte_kvargs
Checks
Commit Message
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
Cc: Neil Horman <nhorman@tuxdriver.com>
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(-)
Comments
On Fri, Mar 23, 2018 at 07:45:03PM +0100, Gaetan Rivet wrote:
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>
> Cc: Neil Horman <nhorman@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
>
>
Hi Neil,
On Mon, Mar 26, 2018 at 07:38:19AM -0400, Neil Horman wrote:
> On Fri, Mar 23, 2018 at 07:45:03PM +0100, Gaetan Rivet wrote:
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >
> > Cc: Neil Horman <nhorman@tuxdriver.com>
> >
Keep in mind that all of this is to achieve the trivial task I was
doing in 20 lines or so.
> 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
>
I don't need a recursive construct or a tree-like structure. I only need
an alternative to '\0' to signify "end-of-list".
This seems like an edge-case to librte_kvargs that would only be useful
to a specific case. It does not seem a wise addition.
So maybe I did not understand your suggestion. Can you give an example
of inputs?
I need to parse something like
"bus=pci,vendor_id=0x8086/class=eth"
(and I only care about bus=pci and class=eth).
how can grouping help? My issue is that librte_kvargs would parse
key:vendor_id
value:0x8086/class
and would then stumble on the unexpected '='.
> 3) Layer your functionality on top of (1) and (2), as Keith noted
The stack allocator seems like a nice-to-have that would interest
people using librte_kvargs. I find librte_kvargs to be cumbersome. I
cannot rewrite it from scratch, unless I update everything that relies
on it as well. So I do not touch it, because I don't care *that* much.
Why not simply leave my helper alongside? If people care enough about
it and would prefer to use it over librte_kvargs, then maybe we could
think about doing the effort of exposing it cleanly (or maybe they could).
Right now, I see only me needing it and I do not see this effort as
worth it.
Regards,
> On Mar 26, 2018, at 8:59 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>
> Hi Neil,
>
> On Mon, Mar 26, 2018 at 07:38:19AM -0400, Neil Horman wrote:
>> On Fri, Mar 23, 2018 at 07:45:03PM +0100, Gaetan Rivet wrote:
>>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>>> ---
>>>
>>> Cc: Neil Horman <nhorman@tuxdriver.com>
>>>
>
> Keep in mind that all of this is to achieve the trivial task I was
> doing in 20 lines or so.
>
>> 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
>>
>
> I don't need a recursive construct or a tree-like structure. I only need
> an alternative to '\0' to signify "end-of-list".
> This seems like an edge-case to librte_kvargs that would only be useful
> to a specific case. It does not seem a wise addition.
>
> So maybe I did not understand your suggestion. Can you give an example
> of inputs?
>
> I need to parse something like
>
> "bus=pci,vendor_id=0x8086/class=eth"
> (and I only care about bus=pci and class=eth).
>
> how can grouping help? My issue is that librte_kvargs would parse
>
> key:vendor_id
> value:0x8086/class
>
> and would then stumble on the unexpected '=‘.
Let me try to remember what I did here.
Created a new list structure of top level keywords like ‘bus’, ‘class’, …
The new list is passed into the new API that splits up the string by ‘/‘ then for each string you call into the original kvargs routines to parse the remaining items within each string. I believe I passed a function pointer in the new array of structures that allowed me to handle the top level keywords. What is passed to the top level keyword function is the string to be parsed by the normal kvargs routines. Each routine then used kvargs routines as normal. I am sure the layer could be done differently and the structure could maybe even contain the kvargs list or keywords if you want.
Anyway it worked out pretty well, just adding new APIs to handle and layer on top of kvargs APIs.
>
>> 3) Layer your functionality on top of (1) and (2), as Keith noted
>
> The stack allocator seems like a nice-to-have that would interest
> people using librte_kvargs. I find librte_kvargs to be cumbersome. I
> cannot rewrite it from scratch, unless I update everything that relies
> on it as well. So I do not touch it, because I don't care *that* much.
>
> Why not simply leave my helper alongside? If people care enough about
> it and would prefer to use it over librte_kvargs, then maybe we could
> think about doing the effort of exposing it cleanly (or maybe they could).
> Right now, I see only me needing it and I do not see this effort as
> worth it.
>
> Regards,
> --
> Gaëtan Rivet
> 6WIND
Regards,
Keith
@@ -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;
}
@@ -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