[dpdk-dev,v4,09/20] eal/dev: implement device iteration initialization
Checks
Commit Message
Parse a device description.
Split this description in their relevant part for each layers.
No dynamic allocation is performed.
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
lib/Makefile | 1 +
lib/librte_eal/bsdapp/eal/Makefile | 1 +
lib/librte_eal/common/eal_common_dev.c | 147 ++++++++++++++++++++++++++++++++
lib/librte_eal/common/include/rte_dev.h | 23 +++++
lib/librte_eal/linuxapp/eal/Makefile | 1 +
lib/librte_eal/rte_eal_version.map | 1 +
6 files changed, 174 insertions(+)
Comments
> On Mar 29, 2018, at 4:23 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>
> Parse a device description.
> Split this description in their relevant part for each layers.
> No dynamic allocation is performed.
>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
> lib/Makefile | 1 +
> lib/librte_eal/bsdapp/eal/Makefile | 1 +
> lib/librte_eal/common/eal_common_dev.c | 147 ++++++++++++++++++++++++++++++++
> lib/librte_eal/common/include/rte_dev.h | 23 +++++
> lib/librte_eal/linuxapp/eal/Makefile | 1 +
> lib/librte_eal/rte_eal_version.map | 1 +
> 6 files changed, 174 insertions(+)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index fc7a55a37..1b17526f7 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -6,6 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> DIRS-y += librte_compat
> DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
> DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
> +DEPDIRS-librte_eal := librte_kvargs
> DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci
> DEPDIRS-librte_pci := librte_eal
> DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
> diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
> index 17ff1ac45..f6cea7fc2 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile
> @@ -18,6 +18,7 @@ CFLAGS += $(WERROR_FLAGS) -O3
> LDLIBS += -lexecinfo
> LDLIBS += -lpthread
> LDLIBS += -lgcc_s
> +LDLIBS += -lrte_kvargs
>
> EXPORT_MAP := ../../rte_eal_version.map
>
> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index cd071442f..1f6df2351 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -10,9 +10,12 @@
>
> #include <rte_compat.h>
> #include <rte_bus.h>
> +#include <rte_class.h>
> #include <rte_dev.h>
> #include <rte_devargs.h>
> #include <rte_debug.h>
> +#include <rte_errno.h>
> +#include <rte_kvargs.h>
> #include <rte_log.h>
>
> #include "eal_private.h"
> @@ -207,3 +210,147 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
> rte_eal_devargs_remove(busname, devname);
> return ret;
> }
> +
> +static size_t
> +dev_layer_count(const char *s)
> +{
> + size_t i = s ? 1 : 0;
> +
> + while (s != NULL && s[0] != '\0') {
> + i += s[0] == '/';
> + s++;
> + }
> + return i;
> +}
> +
> +int __rte_experimental
> +rte_dev_iterator_init(struct rte_dev_iterator *it,
> + const char *devstr)
> +{
> + struct {
> + const char *key;
> + const char *str;
> + struct rte_kvargs *kvlist;
> + } layers[] = {
> + { "bus=", NULL, NULL, },
> + { "class=", NULL, NULL, },
> + { "driver=", NULL, NULL, },
> + };
> + struct rte_kvargs_pair *kv = NULL;
> + struct rte_class *cls = NULL;
> + struct rte_bus *bus = NULL;
> + const char *s = devstr;
> + size_t nblayer;
> + size_t i = 0;
> +
> + /* Having both busstr and clsstr NULL is illegal,
> + * marking this iterator as invalid unless
> + * everything goes well.
> + */
> + it->busstr = NULL;
> + it->clsstr = NULL;
> + /* Split each sub-lists. */
> + nblayer = dev_layer_count(devstr);
> + if (nblayer > RTE_DIM(layers)) {
> + RTE_LOG(ERR, EAL, "Invalid query: too many layers (%zu)\n",
> + nblayer);
> + rte_errno = EINVAL;
> + goto get_out;
This one could just return -EINVAL; instead of calling get_out.
> + }
> + while (s != NULL) {
> + char *copy;
> + char *end;
> +
> + if (strncmp(layers[i].key, s,
> + strlen(layers[i].key)))
> + goto next_layer;
> + layers[i].str = s;
> + copy = strdup(s);
> + if (copy == NULL) {
> + RTE_LOG(ERR, EAL, "OOM\n”);
Maybe spell it out in the log ‘Out of Memory’.
> + rte_errno = ENOMEM;
> + goto get_out;
> + }
> + end = strchr(copy, '/');
> + end = end ? end : strchr(copy, '\0');
> + end[0] = '\0';
> + layers[i].kvlist = rte_kvargs_parse(copy, NULL);
> + free(copy);
I am sorry this method of not adding blank lines is a bit silly and we need to rethink at least adding a few blank lines to help with grouping the code. This style will cause problems for new readers (old readers) to understand the code. This to me is a maintenance problem for the future and we need to fix this now.
Blank lines after if statements (with possible more comments) can help along with adding blank lines to group code is really the minimum amount of lines required. I have never seen someone state you have to many blanks lines in the code except two or more blank lines in a row. This is akin to having a single paragraph in a novel or letter and it makes it very hard to read. It is not hard to add some blank lines to the code for readability.
I stopped reviewing the code as it became difficult to read IMO and just causing me a headache.
This problem needs to be addressed by the TSC IMO.
> + if (layers[i].kvlist == NULL) {
> + RTE_LOG(ERR, EAL, "Could not parse %s\n", s);
> + rte_errno = EINVAL;
> + goto get_out;
> + }
> + s = strchr(s, '/');
> + if (s != NULL)
> + s++;
> +next_layer:
> + if (i >= RTE_DIM(layers)) {
> + RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
> + rte_errno = EINVAL;
> + goto get_out;
> + }
> + i++;
> + }
> + /* Parse each sub-list. */
> + for (i = 0; i < RTE_DIM(layers); i++) {
> + if (layers[i].kvlist == NULL)
> + continue;
> + kv = &layers[i].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);
> + rte_errno = EFAULT;
> + goto get_out;
> + }
> + } 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);
> + rte_errno = EFAULT;
> + goto get_out;
> + }
> + } else if (strcmp(kv->key, "driver") == 0) {
> + /* Ignore */
> + continue;
> + }
> + }
> + /* The string should have at least
> + * one layer specified.
> + */
> + if (bus == NULL && cls == NULL) {
> + RTE_LOG(ERR, EAL,
> + "Either bus or class must be specified.\n");
> + rte_errno = EINVAL;
> + goto get_out;
> + }
> + if (bus != NULL && bus->dev_iterate == NULL) {
> + RTE_LOG(ERR, EAL, "Bus %s not supported\n", bus->name);
> + rte_errno = ENOTSUP;
> + goto get_out;
> + }
> + if (cls != NULL && cls->dev_iterate == NULL) {
> + RTE_LOG(ERR, EAL, "Class %s not supported\n", cls->name);
> + rte_errno = ENOTSUP;
> + goto get_out;
> + }
> + /* Fill iterator fields. */
> + if (bus != NULL)
> + it->busstr = layers[0].str;
> + if (cls != NULL)
> + it->clsstr = layers[1].str;
> + it->devstr = devstr;
> + it->bus = bus;
> + it->cls = cls;
> + it->device = NULL;
> + it->class_device = NULL;
> +get_out:
> + for (i = 0; i < RTE_DIM(layers); i++) {
> + if (layers[i].kvlist)
> + rte_kvargs_free(layers[i].kvlist);
> + }
> + return -rte_errno;
> +}
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index 937ff6079..7ce13e068 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -310,6 +310,29 @@ typedef void *(*rte_dev_iterate_t)(const void *start,
> const char *devstr,
> const struct rte_dev_iterator *it);
>
> +/**
> + * Initializes a device iterator.
> + *
> + * This iterator allows accessing a list of devices matching a criteria.
> + * The device matching is made among all buses and classes currently registered,
> + * filtered by the device description given as parameter.
> + *
> + * This function will not allocate any memory. It is safe to stop the
> + * iteration at any moment and let the iterator go out of context.
> + *
> + * @param it
> + * Device iterator handle.
> + *
> + * @param str
> + * Device description string.
> + *
> + * @return
> + * 0 on successful initialization.
> + * <0 on error.
> + */
> +int __rte_experimental
> +rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str);
> +
> #ifdef __cplusplus
> }
> #endif
> 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
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index 910cb23c9..921da3075 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -228,6 +228,7 @@ EXPERIMENTAL {
> rte_mp_sendmsg;
> rte_mp_request;
> rte_mp_reply;
> + rte_dev_iterator_init;
> rte_service_attr_get;
> rte_service_attr_reset_all;
> rte_service_component_register;
> --
> 2.11.0
>
Regards,
Keith
Hello Keith,
On Fri, Mar 30, 2018 at 03:22:59PM +0000, Wiles, Keith wrote:
>
>
> > On Mar 29, 2018, at 4:23 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >
> > Parse a device description.
> > Split this description in their relevant part for each layers.
> > No dynamic allocation is performed.
> >
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> > lib/Makefile | 1 +
> > lib/librte_eal/bsdapp/eal/Makefile | 1 +
> > lib/librte_eal/common/eal_common_dev.c | 147 ++++++++++++++++++++++++++++++++
> > lib/librte_eal/common/include/rte_dev.h | 23 +++++
> > lib/librte_eal/linuxapp/eal/Makefile | 1 +
> > lib/librte_eal/rte_eal_version.map | 1 +
> > 6 files changed, 174 insertions(+)
> >
> > diff --git a/lib/Makefile b/lib/Makefile
> > index fc7a55a37..1b17526f7 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -6,6 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > DIRS-y += librte_compat
> > DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
> > DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
> > +DEPDIRS-librte_eal := librte_kvargs
> > DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci
> > DEPDIRS-librte_pci := librte_eal
> > DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
> > diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
> > index 17ff1ac45..f6cea7fc2 100644
> > --- a/lib/librte_eal/bsdapp/eal/Makefile
> > +++ b/lib/librte_eal/bsdapp/eal/Makefile
> > @@ -18,6 +18,7 @@ CFLAGS += $(WERROR_FLAGS) -O3
> > LDLIBS += -lexecinfo
> > LDLIBS += -lpthread
> > LDLIBS += -lgcc_s
> > +LDLIBS += -lrte_kvargs
> >
> > EXPORT_MAP := ../../rte_eal_version.map
> >
> > diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> > index cd071442f..1f6df2351 100644
> > --- a/lib/librte_eal/common/eal_common_dev.c
> > +++ b/lib/librte_eal/common/eal_common_dev.c
> > @@ -10,9 +10,12 @@
> >
> > #include <rte_compat.h>
> > #include <rte_bus.h>
> > +#include <rte_class.h>
> > #include <rte_dev.h>
> > #include <rte_devargs.h>
> > #include <rte_debug.h>
> > +#include <rte_errno.h>
> > +#include <rte_kvargs.h>
> > #include <rte_log.h>
> >
> > #include "eal_private.h"
> > @@ -207,3 +210,147 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
> > rte_eal_devargs_remove(busname, devname);
> > return ret;
> > }
> > +
> > +static size_t
> > +dev_layer_count(const char *s)
> > +{
> > + size_t i = s ? 1 : 0;
> > +
> > + while (s != NULL && s[0] != '\0') {
> > + i += s[0] == '/';
> > + s++;
> > + }
> > + return i;
> > +}
> > +
> > +int __rte_experimental
> > +rte_dev_iterator_init(struct rte_dev_iterator *it,
> > + const char *devstr)
> > +{
> > + struct {
> > + const char *key;
> > + const char *str;
> > + struct rte_kvargs *kvlist;
> > + } layers[] = {
> > + { "bus=", NULL, NULL, },
> > + { "class=", NULL, NULL, },
> > + { "driver=", NULL, NULL, },
> > + };
> > + struct rte_kvargs_pair *kv = NULL;
> > + struct rte_class *cls = NULL;
> > + struct rte_bus *bus = NULL;
> > + const char *s = devstr;
> > + size_t nblayer;
> > + size_t i = 0;
> > +
> > + /* Having both busstr and clsstr NULL is illegal,
> > + * marking this iterator as invalid unless
> > + * everything goes well.
> > + */
> > + it->busstr = NULL;
> > + it->clsstr = NULL;
> > + /* Split each sub-lists. */
> > + nblayer = dev_layer_count(devstr);
> > + if (nblayer > RTE_DIM(layers)) {
> > + RTE_LOG(ERR, EAL, "Invalid query: too many layers (%zu)\n",
> > + nblayer);
> > + rte_errno = EINVAL;
> > + goto get_out;
>
> This one could just return -EINVAL; instead of calling get_out.
> > + }
> > + while (s != NULL) {
> > + char *copy;
> > + char *end;
> > +
> > + if (strncmp(layers[i].key, s,
> > + strlen(layers[i].key)))
> > + goto next_layer;
> > + layers[i].str = s;
> > + copy = strdup(s);
> > + if (copy == NULL) {
> > + RTE_LOG(ERR, EAL, "OOM\n”);
>
> Maybe spell it out in the log ‘Out of Memory’.
Will do.
> > + rte_errno = ENOMEM;
> > + goto get_out;
> > + }
> > + end = strchr(copy, '/');
> > + end = end ? end : strchr(copy, '\0');
> > + end[0] = '\0';
> > + layers[i].kvlist = rte_kvargs_parse(copy, NULL);
> > + free(copy);
>
> I am sorry this method of not adding blank lines is a bit silly and we need to rethink at least adding a few blank lines to help with grouping the code. This style will cause problems for new readers (old readers) to understand the code. This to me is a maintenance problem for the future and we need to fix this now.
>
> Blank lines after if statements (with possible more comments) can help along with adding blank lines to group code is really the minimum amount of lines required. I have never seen someone state you have to many blanks lines in the code except two or more blank lines in a row. This is akin to having a single paragraph in a novel or letter and it makes it very hard to read. It is not hard to add some blank lines to the code for readability.
>
I understand. What I dislike is having inconsistencies in the layout of
the code.
"Paragraphs", for lack of a better word, are high subjective and a
matter of taste.
Given that subjectivity is not helpful in review and taste is hard to
debate, I prefer to have a single terse rule, that is to avoid any
additional bytes beside the bare minimum to respect checkpatch.
When I feel the need to have a new "conceptual group", then I am forced
to add a comment to explain what is happening. By not allowing blank
lines, I thus feel the pressure for space and explanation more acutely.
Also, this makes a patch context as information-rich as possible.
> I stopped reviewing the code as it became difficult to read IMO and just causing me a headache.
>
> This problem needs to be addressed by the TSC IMO.
Sorry that it made it more difficult than necessary.
It would be interesting if an addendum to the kernel coding style was
made regarding blank lines.
Regards,
> On Mar 30, 2018, at 10:53 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>
> Hello Keith,
Hello Gaëtan,
>>> + layers[i].kvlist = rte_kvargs_parse(copy, NULL);
>>> + free(copy);
>>
>> I am sorry this method of not adding blank lines is a bit silly and we need to rethink at least adding a few blank lines to help with grouping the code. This style will cause problems for new readers (old readers) to understand the code. This to me is a maintenance problem for the future and we need to fix this now.
>>
>> Blank lines after if statements (with possible more comments) can help along with adding blank lines to group code is really the minimum amount of lines required. I have never seen someone state you have to many blanks lines in the code except two or more blank lines in a row. This is akin to having a single paragraph in a novel or letter and it makes it very hard to read. It is not hard to add some blank lines to the code for readability.
>>
>
> I understand. What I dislike is having inconsistencies in the layout of
> the code.
>
> "Paragraphs", for lack of a better word, are high subjective and a
> matter of taste.
I bet your teacher(s) would disagree with that statement with one single paragraph in your book reports :-)
>
> Given that subjectivity is not helpful in review and taste is hard to
> debate, I prefer to have a single terse rule, that is to avoid any
> additional bytes beside the bare minimum to respect checkpatch.
Taste is hard to debate, but you have gone the extreme route with only the bare minimum blank lines and that is not good as well. A silly script does not read code or understand code we the humans have to make the code readable.
>
> When I feel the need to have a new "conceptual group", then I am forced
> to add a comment to explain what is happening. By not allowing blank
> lines, I thus feel the pressure for space and explanation more acutely.
A blank can give somewhat convey the same information without a comment, but not in all cases. Even a blank before a group of lines can convey this is a new logic section. I do not buy the point for needing the same terse rule here as most of the coding style is free form and allows for reading between the lines a bit.
We can make a rule here, but no blank lines in a function this size and the patch of this size is making DPDK not as professional looking as it could be.
I am not asking for a blank line after every statement, but when I see functions of this size without a blank lines it means we are not addressing readability for future developers to read this code. If you have to put a comment before a section of code then normally a blank line before the comment is reasonable.
Look at the rest of DPDK code and tell me that we do not use blank lines wisely or no blank lines other then what the silly checkpatch script flags.
Below I added a few blank lines not many, but just few to help the reader detect sections and breaks in the code. Even if you added a few lines or removed a few lines I would think this is much more readable in the long run.
(Wish I could remove the ‘+’ it would help to make my point clearer, but afraid email will left justify the code).
+int __rte_experimental
+rte_dev_iterator_init(struct rte_dev_iterator *it,
+ const char *devstr)
+{
+ struct {
+ const char *key;
+ const char *str;
+ struct rte_kvargs *kvlist;
+ } layers[] = {
+ { "bus=", NULL, NULL, },
+ { "class=", NULL, NULL, },
+ { "driver=", NULL, NULL, },
+ };
+ struct rte_kvargs_pair *kv = NULL;
+ struct rte_class *cls = NULL;
+ struct rte_bus *bus = NULL;
+ const char *s = devstr;
+ size_t nblayer;
+ size_t i = 0;
+
+ /* Having both busstr and clsstr NULL is illegal,
+ * marking this iterator as invalid unless
+ * everything goes well.
+ */
+ it->busstr = NULL;
+ it->clsstr = NULL;
+
+ /* Split each sub-lists. */
+ nblayer = dev_layer_count(devstr);
+ if (nblayer > RTE_DIM(layers)) {
+ RTE_LOG(ERR, EAL, "Invalid query: too many layers (%zu)\n",
+ nblayer);
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+
+ while (s != NULL) {
+ char *copy;
+ char *end;
+
+ if (strncmp(layers[i].key, s,
+ strlen(layers[i].key)))
+ goto next_layer;
+
+ layers[i].str = s;
+ copy = strdup(s);
+ if (copy == NULL) {
+ RTE_LOG(ERR, EAL, "OOM\n");
+ rte_errno = ENOMEM;
+ goto get_out;
+ }
+
+ end = strchr(copy, '/');
+ end = end ? end : strchr(copy, '\0');
+ end[0] = '\0';
+ layers[i].kvlist = rte_kvargs_parse(copy, NULL);
+ free(copy);
+
+ if (layers[i].kvlist == NULL) {
+ RTE_LOG(ERR, EAL, "Could not parse %s\n", s);
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+
+ s = strchr(s, '/');
+ if (s != NULL)
+ s++;
+
+next_layer:
+ if (i >= RTE_DIM(layers)) {
+ RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+ i++;
+ }
+
+ /* Parse each sub-list. */
+ for (i = 0; i < RTE_DIM(layers); i++) {
+ if (layers[i].kvlist == NULL)
+ continue;
+
+ kv = &layers[i].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);
+ rte_errno = EFAULT;
+ goto get_out;
+ }
+ } 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);
+ rte_errno = EFAULT;
+ goto get_out;
+ }
+ } else if (strcmp(kv->key, "driver") == 0) {
+ /* Ignore */
+ continue;
+ }
+ }
+
+ /* The string should have at least
+ * one layer specified.
+ */
+ if (bus == NULL && cls == NULL) {
+ RTE_LOG(ERR, EAL,
+ "Either bus or class must be specified.\n");
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+
+ if (bus != NULL && bus->dev_iterate == NULL) {
+ RTE_LOG(ERR, EAL, "Bus %s not supported\n", bus->name);
+ rte_errno = ENOTSUP;
+ goto get_out;
+ }
+
+ if (cls != NULL && cls->dev_iterate == NULL) {
+ RTE_LOG(ERR, EAL, "Class %s not supported\n", cls->name);
+ rte_errno = ENOTSUP;
+ goto get_out;
+ }
+
+ /* Fill iterator fields. */
+ if (bus != NULL)
+ it->busstr = layers[0].str;
+ if (cls != NULL)
+ it->clsstr = layers[1].str;
+
+ it->devstr = devstr;
+ it->bus = bus;
+ it->cls = cls;
+ it->device = NULL;
+ it->class_device = NULL;
+
+get_out:
+ for (i = 0; i < RTE_DIM(layers); i++) {
+ if (layers[i].kvlist)
+ rte_kvargs_free(layers[i].kvlist);
+ }
+
+ return -rte_errno;
+}
>
> Also, this makes a patch context as information-rich as possible.
>
>> I stopped reviewing the code as it became difficult to read IMO and just causing me a headache.
>>
>> This problem needs to be addressed by the TSC IMO.
>
> Sorry that it made it more difficult than necessary.
> It would be interesting if an addendum to the kernel coding style was
> made regarding blank lines.
>
> Regards,
> --
> Gaëtan Rivet
> 6WIND
Regards,
Keith
On Fri, Mar 30, 2018 at 04:22:15PM +0000, Wiles, Keith wrote:
>
>
> > On Mar 30, 2018, at 10:53 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> >
> > Hello Keith,
>
> Hello Gaëtan,
>
> >>> + layers[i].kvlist = rte_kvargs_parse(copy, NULL);
> >>> + free(copy);
> >>
> >> I am sorry this method of not adding blank lines is a bit silly and we need to rethink at least adding a few blank lines to help with grouping the code. This style will cause problems for new readers (old readers) to understand the code. This to me is a maintenance problem for the future and we need to fix this now.
> >>
> >> Blank lines after if statements (with possible more comments) can help along with adding blank lines to group code is really the minimum amount of lines required. I have never seen someone state you have to many blanks lines in the code except two or more blank lines in a row. This is akin to having a single paragraph in a novel or letter and it makes it very hard to read. It is not hard to add some blank lines to the code for readability.
> >>
> >
> > I understand. What I dislike is having inconsistencies in the layout of
> > the code.
> >
> > "Paragraphs", for lack of a better word, are high subjective and a
> > matter of taste.
>
> I bet your teacher(s) would disagree with that statement with one single paragraph in your book reports :-)
The utility of using paragraph is not a matter of subjectivity.
How to use them and how to structure information is what I deemed
subjective.
> >
> > Given that subjectivity is not helpful in review and taste is hard to
> > debate, I prefer to have a single terse rule, that is to avoid any
> > additional bytes beside the bare minimum to respect checkpatch.
>
> Taste is hard to debate, but you have gone the extreme route with only the bare minimum blank lines and that is not good as well. A silly script does not read code or understand code we the humans have to make the code readable.
> >
> > When I feel the need to have a new "conceptual group", then I am forced
> > to add a comment to explain what is happening. By not allowing blank
> > lines, I thus feel the pressure for space and explanation more acutely.
>
> A blank can give somewhat convey the same information without a comment, but not in all cases. Even a blank before a group of lines can convey this is a new logic section. I do not buy the point for needing the same terse rule here as most of the coding style is free form and allows for reading between the lines a bit.
>
> We can make a rule here, but no blank lines in a function this size and the patch of this size is making DPDK not as professional looking as it could be.
>
I think one of the main issue anyway is that this function is simply too
large as it is.
As it turns out, I am working on the second side of this work, which is
revamping the device declaration path.
This function will be split in different parts, that will be shorter and
more palatable.
This is a big work, still in progress (the device declaration, not the
split of this function).
@@ -6,6 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
DIRS-y += librte_compat
DIRS-$(CONFIG_RTE_LIBRTE_KVARGS) += librte_kvargs
DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
+DEPDIRS-librte_eal := librte_kvargs
DIRS-$(CONFIG_RTE_LIBRTE_PCI) += librte_pci
DEPDIRS-librte_pci := librte_eal
DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
@@ -18,6 +18,7 @@ CFLAGS += $(WERROR_FLAGS) -O3
LDLIBS += -lexecinfo
LDLIBS += -lpthread
LDLIBS += -lgcc_s
+LDLIBS += -lrte_kvargs
EXPORT_MAP := ../../rte_eal_version.map
@@ -10,9 +10,12 @@
#include <rte_compat.h>
#include <rte_bus.h>
+#include <rte_class.h>
#include <rte_dev.h>
#include <rte_devargs.h>
#include <rte_debug.h>
+#include <rte_errno.h>
+#include <rte_kvargs.h>
#include <rte_log.h>
#include "eal_private.h"
@@ -207,3 +210,147 @@ rte_eal_hotplug_remove(const char *busname, const char *devname)
rte_eal_devargs_remove(busname, devname);
return ret;
}
+
+static size_t
+dev_layer_count(const char *s)
+{
+ size_t i = s ? 1 : 0;
+
+ while (s != NULL && s[0] != '\0') {
+ i += s[0] == '/';
+ s++;
+ }
+ return i;
+}
+
+int __rte_experimental
+rte_dev_iterator_init(struct rte_dev_iterator *it,
+ const char *devstr)
+{
+ struct {
+ const char *key;
+ const char *str;
+ struct rte_kvargs *kvlist;
+ } layers[] = {
+ { "bus=", NULL, NULL, },
+ { "class=", NULL, NULL, },
+ { "driver=", NULL, NULL, },
+ };
+ struct rte_kvargs_pair *kv = NULL;
+ struct rte_class *cls = NULL;
+ struct rte_bus *bus = NULL;
+ const char *s = devstr;
+ size_t nblayer;
+ size_t i = 0;
+
+ /* Having both busstr and clsstr NULL is illegal,
+ * marking this iterator as invalid unless
+ * everything goes well.
+ */
+ it->busstr = NULL;
+ it->clsstr = NULL;
+ /* Split each sub-lists. */
+ nblayer = dev_layer_count(devstr);
+ if (nblayer > RTE_DIM(layers)) {
+ RTE_LOG(ERR, EAL, "Invalid query: too many layers (%zu)\n",
+ nblayer);
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+ while (s != NULL) {
+ char *copy;
+ char *end;
+
+ if (strncmp(layers[i].key, s,
+ strlen(layers[i].key)))
+ goto next_layer;
+ layers[i].str = s;
+ copy = strdup(s);
+ if (copy == NULL) {
+ RTE_LOG(ERR, EAL, "OOM\n");
+ rte_errno = ENOMEM;
+ goto get_out;
+ }
+ end = strchr(copy, '/');
+ end = end ? end : strchr(copy, '\0');
+ end[0] = '\0';
+ layers[i].kvlist = rte_kvargs_parse(copy, NULL);
+ free(copy);
+ if (layers[i].kvlist == NULL) {
+ RTE_LOG(ERR, EAL, "Could not parse %s\n", s);
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+ s = strchr(s, '/');
+ if (s != NULL)
+ s++;
+next_layer:
+ if (i >= RTE_DIM(layers)) {
+ RTE_LOG(ERR, EAL, "Unrecognized layer %s\n", s);
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+ i++;
+ }
+ /* Parse each sub-list. */
+ for (i = 0; i < RTE_DIM(layers); i++) {
+ if (layers[i].kvlist == NULL)
+ continue;
+ kv = &layers[i].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);
+ rte_errno = EFAULT;
+ goto get_out;
+ }
+ } 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);
+ rte_errno = EFAULT;
+ goto get_out;
+ }
+ } else if (strcmp(kv->key, "driver") == 0) {
+ /* Ignore */
+ continue;
+ }
+ }
+ /* The string should have at least
+ * one layer specified.
+ */
+ if (bus == NULL && cls == NULL) {
+ RTE_LOG(ERR, EAL,
+ "Either bus or class must be specified.\n");
+ rte_errno = EINVAL;
+ goto get_out;
+ }
+ if (bus != NULL && bus->dev_iterate == NULL) {
+ RTE_LOG(ERR, EAL, "Bus %s not supported\n", bus->name);
+ rte_errno = ENOTSUP;
+ goto get_out;
+ }
+ if (cls != NULL && cls->dev_iterate == NULL) {
+ RTE_LOG(ERR, EAL, "Class %s not supported\n", cls->name);
+ rte_errno = ENOTSUP;
+ goto get_out;
+ }
+ /* Fill iterator fields. */
+ if (bus != NULL)
+ it->busstr = layers[0].str;
+ if (cls != NULL)
+ it->clsstr = layers[1].str;
+ it->devstr = devstr;
+ it->bus = bus;
+ it->cls = cls;
+ it->device = NULL;
+ it->class_device = NULL;
+get_out:
+ for (i = 0; i < RTE_DIM(layers); i++) {
+ if (layers[i].kvlist)
+ rte_kvargs_free(layers[i].kvlist);
+ }
+ return -rte_errno;
+}
@@ -310,6 +310,29 @@ typedef void *(*rte_dev_iterate_t)(const void *start,
const char *devstr,
const struct rte_dev_iterator *it);
+/**
+ * Initializes a device iterator.
+ *
+ * This iterator allows accessing a list of devices matching a criteria.
+ * The device matching is made among all buses and classes currently registered,
+ * filtered by the device description given as parameter.
+ *
+ * This function will not allocate any memory. It is safe to stop the
+ * iteration at any moment and let the iterator go out of context.
+ *
+ * @param it
+ * Device iterator handle.
+ *
+ * @param str
+ * Device description string.
+ *
+ * @return
+ * 0 on successful initialization.
+ * <0 on error.
+ */
+int __rte_experimental
+rte_dev_iterator_init(struct rte_dev_iterator *it, const char *str);
+
#ifdef __cplusplus
}
#endif
@@ -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
@@ -228,6 +228,7 @@ EXPERIMENTAL {
rte_mp_sendmsg;
rte_mp_request;
rte_mp_reply;
+ rte_dev_iterator_init;
rte_service_attr_get;
rte_service_attr_reset_all;
rte_service_component_register;