[v10,08/27] devargs: add function to parse device layers

Message ID 0a2d77e22658987a6e1e650913f020a140c2c1a9.1530791217.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Device querying |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Gaëtan Rivet July 5, 2018, 11:48 a.m. UTC
  This function is private to the EAL.
It is used to parse each layers in a device description string,
and store the result in an rte_devargs structure.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_devargs.c  | 144 ++++++++++++++++++++
 lib/librte_eal/common/eal_private.h         |  27 ++++
 lib/librte_eal/common/include/rte_devargs.h |  13 +-
 3 files changed, 181 insertions(+), 3 deletions(-)
  

Comments

Thomas Monjalon July 11, 2018, 8:19 a.m. UTC | #1
05/07/2018 13:48, Gaetan Rivet:
> +/**
> + * @internal
> + * Parse a device string and store its information in an
> + * rte_devargs structure.

Please, explain what is a layer.

> + *
> + * Note: if the "data" field of da points to devstr,

Better to use "devargs" as variable name, instead of "da".

> + * then no dynamic allocation is performed and the rte_devargs
> + * can be safely discarded.
> + *
> + * Otherwise ``data`` will hold a workable copy of devstr, that will be
> + * used by layers descriptors within rte_devargs. In this case,
> + * any rte_devargs should be cleaned-up before being freed.
> + *
> + * @param da
> + *   rte_devargs structure to fill.
> + *
> + * @param devstr
> + *   Device string.
> + *
> + * @return
> + *   0 on success.
> + *   Negative errno values on error (rte_errno is set).
> + */
> +int
> +rte_devargs_layers_parse(struct rte_devargs *da,
> +			 const char *devstr);
> +
>  #endif /* _EAL_PRIVATE_H_ */
> diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> index 6c3b6326b..148600258 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -51,12 +51,19 @@ struct rte_devargs {
>  	enum rte_devtype type;
>  	/** Device policy. */
>  	enum rte_dev_policy policy;
> -	/** Bus handle for the device. */
> -	struct rte_bus *bus;
>  	/** Name of the device. */
>  	char name[RTE_DEV_NAME_MAX_LEN];
> +	RTE_STD_C11
> +	union {
>  	/** Arguments string as given by user or "" for no argument. */
> -	char *args;
> +		char *args;
> +		const char *drvstr;
> +	};
> +	struct rte_bus *bus; /**< bus handle. */
> +	struct rte_class *cls; /**< class handle. */

"class" is more readable than "cls"

> +	const char *busstr; /**< bus-related part of device string. */

bus_str ?

> +	const char *clsstr; /**< bus-related part of device string. */

class_str ?
+ there is a typo in the comment (copy/pasted "bus")

> +	const char *data; /**< Device string storage. */
>  };
  
Gaëtan Rivet July 11, 2018, 8:41 a.m. UTC | #2
Hi Thomas,

On Wed, Jul 11, 2018 at 10:19:07AM +0200, Thomas Monjalon wrote:
> 05/07/2018 13:48, Gaetan Rivet:
> > +/**
> > + * @internal
> > + * Parse a device string and store its information in an
> > + * rte_devargs structure.
> 
> Please, explain what is a layer.
> 
> > + *
> > + * Note: if the "data" field of da points to devstr,
> 
> Better to use "devargs" as variable name, instead of "da".
> 
> > + * then no dynamic allocation is performed and the rte_devargs
> > + * can be safely discarded.
> > + *
> > + * Otherwise ``data`` will hold a workable copy of devstr, that will be
> > + * used by layers descriptors within rte_devargs. In this case,
> > + * any rte_devargs should be cleaned-up before being freed.
> > + *
> > + * @param da
> > + *   rte_devargs structure to fill.
> > + *
> > + * @param devstr
> > + *   Device string.
> > + *
> > + * @return
> > + *   0 on success.
> > + *   Negative errno values on error (rte_errno is set).
> > + */
> > +int
> > +rte_devargs_layers_parse(struct rte_devargs *da,
> > +			 const char *devstr);
> > +
> >  #endif /* _EAL_PRIVATE_H_ */
> > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> > index 6c3b6326b..148600258 100644
> > --- a/lib/librte_eal/common/include/rte_devargs.h
> > +++ b/lib/librte_eal/common/include/rte_devargs.h
> > @@ -51,12 +51,19 @@ struct rte_devargs {
> >  	enum rte_devtype type;
> >  	/** Device policy. */
> >  	enum rte_dev_policy policy;
> > -	/** Bus handle for the device. */
> > -	struct rte_bus *bus;
> >  	/** Name of the device. */
> >  	char name[RTE_DEV_NAME_MAX_LEN];
> > +	RTE_STD_C11
> > +	union {
> >  	/** Arguments string as given by user or "" for no argument. */
> > -	char *args;
> > +		char *args;
> > +		const char *drvstr;
> > +	};
> > +	struct rte_bus *bus; /**< bus handle. */
> > +	struct rte_class *cls; /**< class handle. */
> 
> "class" is more readable than "cls"
> 

I was thinking that maybe this could be a problem when trying to build
with C++. The EAL headers in DPDK are meant to be included in C++ apps,
I think ``class`` would be an issue then.

If I'm mistaken, then sure, class is a better name.

> > +	const char *busstr; /**< bus-related part of device string. */
> 
> bus_str ?
> 
> > +	const char *clsstr; /**< bus-related part of device string. */
> 
> class_str ?
> + there is a typo in the comment (copy/pasted "bus")
> 
> > +	const char *data; /**< Device string storage. */
> >  };
> 
> 
> 

Otherwise, ok with the rest of the comments, will fix (as well as the
previous emails). Thanks for reading.
  
Thomas Monjalon July 11, 2018, 9:30 a.m. UTC | #3
11/07/2018 10:41, Gaëtan Rivet:
> > > +	struct rte_bus *bus; /**< bus handle. */
> > > +	struct rte_class *cls; /**< class handle. */
> > 
> > "class" is more readable than "cls"
> > 
> 
> I was thinking that maybe this could be a problem when trying to build
> with C++. The EAL headers in DPDK are meant to be included in C++ apps,
> I think ``class`` would be an issue then.

Yes, right.

So only options are "cls" or "devclass".
Up to you.

> If I'm mistaken, then sure, class is a better name.
  

Patch

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 0a83beb94..5f89e0b6e 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -13,9 +13,13 @@ 
 #include <string.h>
 #include <stdarg.h>
 
+#include <rte_bus.h>
+#include <rte_class.h>
 #include <rte_compat.h>
 #include <rte_dev.h>
 #include <rte_devargs.h>
+#include <rte_kvargs.h>
+#include <rte_errno.h>
 #include <rte_tailq.h>
 #include "eal_private.h"
 
@@ -56,6 +60,146 @@  rte_eal_parse_devargs_str(const char *devargs_str,
 	return 0;
 }
 
+static size_t
+devargs_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_devargs_layers_parse(struct rte_devargs *da,
+			 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;
+	int ret = 0;
+
+	/* Split each sub-lists. */
+	nblayer = devargs_layer_count(devstr);
+	if (nblayer > RTE_DIM(layers)) {
+		RTE_LOG(ERR, EAL, "Invalid format: too many layers (%zu)\n",
+			nblayer);
+		ret = -E2BIG;
+		goto get_out;
+	}
+
+	/* If the devargs points the devstr
+	 * as source data, then it should not allocate
+	 * anything and keep referring only to it.
+	 */
+	if (da->data != devstr) {
+		da->data = strdup(devstr);
+		if (da->data == NULL) {
+			RTE_LOG(ERR, EAL, "OOM\n");
+			ret = -ENOMEM;
+			goto get_out;
+		}
+		s = da->data;
+	}
+
+	while (s != NULL) {
+		if (strncmp(layers[i].key, s,
+			    strlen(layers[i].key)) &&
+		    /* The last layer is free-form.
+		     * The "driver" key is not required (but accepted).
+		     */
+		    i != RTE_DIM(layers) - 1)
+			goto next_layer;
+		layers[i].str = s;
+		layers[i].kvlist = rte_kvargs_parse2(s, NULL, "/");
+		if (layers[i].kvlist == NULL) {
+			RTE_LOG(ERR, EAL, "Could not parse %s\n", s);
+			ret = -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);
+			ret = -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);
+				ret = -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);
+				ret = -EFAULT;
+				goto get_out;
+			}
+		} else if (strcmp(kv->key, "driver") == 0) {
+			/* Ignore */
+			continue;
+		}
+	}
+
+	/* Fill devargs fields. */
+	da->busstr = layers[0].str;
+	da->clsstr = layers[1].str;
+	da->drvstr = layers[2].str;
+	da->bus = bus;
+	da->cls = cls;
+
+	/* If we own the data, clean up a bit
+	 * the several layers string, to ease
+	 * their parsing afterward.
+	 */
+	if (da->data != devstr) {
+		char *s = (void *)(intptr_t)(da->data);
+
+		while ((s = strchr(s, '/'))) {
+			*s = '\0';
+			s++;
+		}
+	}
+
+get_out:
+	for (i = 0; i < RTE_DIM(layers); i++) {
+		if (layers[i].kvlist)
+			rte_kvargs_free(layers[i].kvlist);
+	}
+	if (ret != 0)
+		rte_errno = -ret;
+	return ret;
+}
+
 static int
 bus_name_cmp(const struct rte_bus *bus, const void *name)
 {
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index bdadc4d50..c4c9283c8 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -258,4 +258,31 @@  int rte_mp_channel_init(void);
  */
 void dev_callback_process(char *device_name, enum rte_dev_event_type event);
 
+/**
+ * @internal
+ * Parse a device string and store its information in an
+ * rte_devargs structure.
+ *
+ * Note: if the "data" field of da points to devstr,
+ * then no dynamic allocation is performed and the rte_devargs
+ * can be safely discarded.
+ *
+ * Otherwise ``data`` will hold a workable copy of devstr, that will be
+ * used by layers descriptors within rte_devargs. In this case,
+ * any rte_devargs should be cleaned-up before being freed.
+ *
+ * @param da
+ *   rte_devargs structure to fill.
+ *
+ * @param devstr
+ *   Device string.
+ *
+ * @return
+ *   0 on success.
+ *   Negative errno values on error (rte_errno is set).
+ */
+int
+rte_devargs_layers_parse(struct rte_devargs *da,
+			 const char *devstr);
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 6c3b6326b..148600258 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -51,12 +51,19 @@  struct rte_devargs {
 	enum rte_devtype type;
 	/** Device policy. */
 	enum rte_dev_policy policy;
-	/** Bus handle for the device. */
-	struct rte_bus *bus;
 	/** Name of the device. */
 	char name[RTE_DEV_NAME_MAX_LEN];
+	RTE_STD_C11
+	union {
 	/** Arguments string as given by user or "" for no argument. */
-	char *args;
+		char *args;
+		const char *drvstr;
+	};
+	struct rte_bus *bus; /**< bus handle. */
+	struct rte_class *cls; /**< class handle. */
+	const char *busstr; /**< bus-related part of device string. */
+	const char *clsstr; /**< bus-related part of device string. */
+	const char *data; /**< Device string storage. */
 };
 
 /**