[PATCH 0/2] Add support for driver directories

[PATCH 0/2] Add support for driver directories

[Panu Matilainen]

This mini-series adds support for driver directory concept as
described by Thomas Monjalon back in February:
http://dpdk.org/ml/archives/dev/2015-February/013285.html

Panu Matilainen (2):
  eal: refactor plugin list append from eal_parse_args() to a helper
    function
  eal: add support for driver directory concept

 config/common_linuxapp                     |  3 ++
 lib/librte_eal/common/eal_common_options.c |  1 +
 lib/librte_eal/linuxapp/eal/eal.c          | 76 ++++++++++++++++++++++++++----
 3 files changed, 71 insertions(+), 9 deletions(-)

-- 
2.4.3

[PATCH 1/2] eal: refactor plugin list append from eal_parse_args() to a helper function

[Panu Matilainen]

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 33e1067..31f3915 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -530,6 +530,24 @@ eal_log_level_parse(int argc, char **argv)
 	optind = 0; /* reset getopt lib */
 }
 
+static int
+eal_add_solib(const char *path)
+{
+	struct shared_driver *solib;
+
+	solib = malloc(sizeof(*solib));
+	if (solib == NULL) {
+		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+		return -1;
+	}
+	memset(solib, 0, sizeof(*solib));
+	strncpy(solib->name, path, PATH_MAX-1);
+	solib->name[PATH_MAX-1] = 0;
+	TAILQ_INSERT_TAIL(&solib_list, solib, next);
+
+	return 0;
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -538,7 +556,6 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	struct shared_driver *solib;
 
 	argvopt = argv;
 
@@ -570,15 +587,8 @@ eal_parse_args(int argc, char **argv)
 
 		/* force loading of external driver */
 		case 'd':
-			solib = malloc(sizeof(*solib));
-			if (solib == NULL) {
-				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+			if (eal_add_solib(optarg) == -1)
 				return -1;
-			}
-			memset(solib, 0, sizeof(*solib));
-			strncpy(solib->name, optarg, PATH_MAX-1);
-			solib->name[PATH_MAX-1] = 0;
-			TAILQ_INSERT_TAIL(&solib_list, solib, next);
 			break;
 
 		/* long options */
-- 
2.4.3

[PATCH 2/2] eal: add support for driver directory concept

[Panu Matilainen]

Add a new EAL option -D for loading all drivers from a given directory.
Additionally a default driver directory can be set in build-time
configuration, in which case it will be always be used when EAL is
initialized (but can be overridden or disabled with -D).

This simplifies usage in shared library configuration significantly over
manually loading individual drivers with -d, and allows distros to
establish a drop-in driver directory for seamless integration
with 3rd party drivers etc.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 config/common_linuxapp                     |  3 ++
 lib/librte_eal/common/eal_common_options.c |  1 +
 lib/librte_eal/linuxapp/eal/eal.c          | 48 ++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0de43d5..a0d8cd2 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -111,6 +111,9 @@ CONFIG_RTE_EAL_IGB_UIO=y
 CONFIG_RTE_EAL_VFIO=y
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # Special configurations in PCI Config Space for high performance
 #
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1f459ac..4dba608 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -58,6 +58,7 @@ eal_short_options[] =
 	"b:" /* pci-blacklist */
 	"c:" /* coremask */
 	"d:" /* driver */
+	"D:" /* driver directory */
 	"h"  /* help */
 	"l:" /* corelist */
 	"m:" /* memory size */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 31f3915..bbb02f0 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -50,6 +50,8 @@
 #include <errno.h>
 #include <sys/mman.h>
 #include <sys/queue.h>
+#include <sys/types.h>
+#include <dirent.h>
 #if defined(RTE_ARCH_X86_64) || defined(RTE_ARCH_I686)
 #include <sys/io.h>
 #endif
@@ -104,6 +106,9 @@ struct shared_driver {
 static struct shared_driver_list solib_list =
 TAILQ_HEAD_INITIALIZER(solib_list);
 
+/* Path of external loadable drivers */
+static const char *solib_dir = RTE_EAL_SOLIB_PATH;
+
 /* early configuration structure, when memory config is not mmapped */
 static struct rte_mem_config early_mem_config;
 
@@ -351,6 +356,7 @@ eal_usage(const char *prgname)
 	eal_common_usage();
 	printf("EAL Linux options:\n"
 	       "  -d LIB.so           Add driver (can be used multiple times)\n"
+	       "  -D DIRECTORY        Add driver directory\n"
 	       "  --"OPT_SOCKET_MEM"        Memory to allocate on sockets (comma separated values)\n"
 	       "  --"OPT_HUGE_DIR"          Directory where hugetlbfs is mounted\n"
 	       "  --"OPT_FILE_PREFIX"       Prefix for hugepage filenames\n"
@@ -548,6 +554,40 @@ eal_add_solib(const char *path)
 	return 0;
 }
 
+static int
+eal_add_solib_dir(const char *path)
+{
+	DIR *d = NULL;
+	struct dirent *dent = NULL;
+	char sopath[PATH_MAX];
+
+	if (path == NULL || *path == '\0')
+		return 0;
+
+	d = opendir(path);
+	if (d == NULL) {
+		RTE_LOG(ERR, EAL, "failed to open directory %s: %s\n",
+			path, strerror(errno));
+		return -1;
+	}
+
+	while ((dent = readdir(d)) != NULL) {
+		if (dent->d_type != DT_REG && dent->d_type != DT_LNK)
+			continue;
+
+		snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name);
+		sopath[PATH_MAX-1] = 0;
+
+		if (eal_add_solib(sopath) == -1)
+			break;
+	}
+
+	closedir(d);
+
+	/* XXX this ignores failures from readdir() itself */
+	return (dent == NULL) ? 0 : -1;
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -591,6 +631,11 @@ eal_parse_args(int argc, char **argv)
 				return -1;
 			break;
 
+		/* set external driver directory */
+		case 'D':
+			solib_dir = optarg;
+			break;
+
 		/* long options */
 		case OPT_XEN_DOM0_NUM:
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -837,6 +882,9 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
+	if (eal_add_solib_dir(solib_dir) == -1)
+		rte_panic("Cannot init plugin directory %s\n", solib_dir);
+
 	TAILQ_FOREACH(solib, &solib_list, next) {
 		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
 		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-- 
2.4.3

Re: [PATCH 0/2] Add support for driver directories

[David Marchand]

Hello Panu,

On Fri, Sep 25, 2015 at 1:58 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:

> This mini-series adds support for driver directory concept as
> described by Thomas Monjalon back in February:
> http://dpdk.org/ml/archives/dev/2015-February/013285.html
>
> Panu Matilainen (2):
>   eal: refactor plugin list append from eal_parse_args() to a helper
>     function
>   eal: add support for driver directory concept
>
>  config/common_linuxapp                     |  3 ++
>  lib/librte_eal/common/eal_common_options.c |  1 +
>  lib/librte_eal/linuxapp/eal/eal.c          | 76
> ++++++++++++++++++++++++++----
>  3 files changed, 71 insertions(+), 9 deletions(-)
>

This patchset looks good to me.
Is there any reason why we should only have this for linux ?


-- 
David Marchand

Re: [PATCH 0/2] Add support for driver directories

[Panu Matilainen]

On 09/25/2015 03:35 PM, David Marchand wrote:
> Hello Panu,
>
> On Fri, Sep 25, 2015 at 1:58 PM, Panu Matilainen <pmatilai@redhat.com
> <mailto:pmatilai@redhat.com>> wrote:
>
>     This mini-series adds support for driver directory concept as
>     described by Thomas Monjalon back in February:
>     http://dpdk.org/ml/archives/dev/2015-February/013285.html
>
>     Panu Matilainen (2):
>        eal: refactor plugin list append from eal_parse_args() to a helper
>          function
>        eal: add support for driver directory concept
>
>       config/common_linuxapp                     |  3 ++
>       lib/librte_eal/common/eal_common_options.c |  1 +
>       lib/librte_eal/linuxapp/eal/eal.c          | 76
>     ++++++++++++++++++++++++++----
>       3 files changed, 71 insertions(+), 9 deletions(-)
>
>
> This patchset looks good to me.
> Is there any reason why we should only have this for linux ?

I merely followed suite with the related, pre-existing -d option which 
also only exists for Linux. And wondered about the same thing :)

	- Panu -

Re: [PATCH 0/2] Add support for driver directories

[Panu Matilainen]

On 09/25/2015 04:00 PM, Panu Matilainen wrote:
> On 09/25/2015 03:35 PM, David Marchand wrote:
>> Hello Panu,
>>
>> On Fri, Sep 25, 2015 at 1:58 PM, Panu Matilainen <pmatilai@redhat.com
>> <mailto:pmatilai@redhat.com>> wrote:
>>
>>     This mini-series adds support for driver directory concept as
>>     described by Thomas Monjalon back in February:
>>     http://dpdk.org/ml/archives/dev/2015-February/013285.html
>>
>>     Panu Matilainen (2):
>>        eal: refactor plugin list append from eal_parse_args() to a helper
>>          function
>>        eal: add support for driver directory concept
>>
>>       config/common_linuxapp                     |  3 ++
>>       lib/librte_eal/common/eal_common_options.c |  1 +
>>       lib/librte_eal/linuxapp/eal/eal.c          | 76
>>     ++++++++++++++++++++++++++----
>>       3 files changed, 71 insertions(+), 9 deletions(-)
>>
>>
>> This patchset looks good to me.
>> Is there any reason why we should only have this for linux ?
>
> I merely followed suite with the related, pre-existing -d option which
> also only exists for Linux. And wondered about the same thing :)

So...

Is this blocking on being Linux-only? I can have a look at moving the 
initialization to common/ if needed, but I wont be able test on FreeBSD.
I'd just like to get this functionality into DPDK 2.2 to remove one of 
those silly usability obstacles thats all :)

	- Panu -

Re: [PATCH 0/2] Add support for driver directories

[David Marchand]

On Wed, Oct 14, 2015 at 12:41 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:

> On 09/25/2015 04:00 PM, Panu Matilainen wrote:
>
>> On 09/25/2015 03:35 PM, David Marchand wrote:
>>
>>> Is there any reason why we should only have this for linux ?
>>>
>>
>> I merely followed suite with the related, pre-existing -d option which
>> also only exists for Linux. And wondered about the same thing :)
>>
>
> So...
>
> Is this blocking on being Linux-only? I can have a look at moving the
> initialization to common/ if needed, but I wont be able test on FreeBSD.
> I'd just like to get this functionality into DPDK 2.2 to remove one of
> those silly usability obstacles thats all :)


Yes this would make it simpler.

Not blocking, but if you can move this to common code, then all better.
I am pretty sure someone can test this on FreeBSD, Bruce maybe ? :-)


-- 
David Marchand

[PATCH 0/5 v2] Add support for driver directories

[Panu Matilainen]

This series adds support for driver directory concept
based on idea by Thomas Monjalon back in February:
http://dpdk.org/ml/archives/dev/2015-February/013285.html

In the process FreeBSD also gains plugin support (but untested).

Panu Matilainen (5):
  eal: refactor plugin list append from eal_parse_args() to a helper
    function
  eal: refactor plugin init from eal_parse_args() to a helper function
  eal: move plugin loading to eal/common
  eal: add an error code to plugin init for the next step
  eal: add support for driver directory concept

 config/common_bsdapp                       |   3 +
 config/common_linuxapp                     |   3 +
 lib/librte_eal/bsdapp/eal/eal.c            |   3 +
 lib/librte_eal/common/eal_common_options.c | 103 +++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |   1 +
 lib/librte_eal/linuxapp/eal/eal.c          |  39 +----------
 6 files changed, 115 insertions(+), 37 deletions(-)

-- 
2.4.3

[PATCH 1/5] eal: refactor plugin list append from eal_parse_args() to a helper function

[Panu Matilainen]

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 33e1067..cc66d9f 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -530,6 +530,24 @@ eal_log_level_parse(int argc, char **argv)
 	optind = 0; /* reset getopt lib */
 }
 
+static int
+eal_plugin_add(const char *path)
+{
+	struct shared_driver *solib;
+
+	solib = malloc(sizeof(*solib));
+	if (solib == NULL) {
+		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+		return -1;
+	}
+	memset(solib, 0, sizeof(*solib));
+	strncpy(solib->name, path, PATH_MAX-1);
+	solib->name[PATH_MAX-1] = 0;
+	TAILQ_INSERT_TAIL(&solib_list, solib, next);
+
+	return 0;
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -538,7 +556,6 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	struct shared_driver *solib;
 
 	argvopt = argv;
 
@@ -570,15 +587,8 @@ eal_parse_args(int argc, char **argv)
 
 		/* force loading of external driver */
 		case 'd':
-			solib = malloc(sizeof(*solib));
-			if (solib == NULL) {
-				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+			if (eal_plugin_add(optarg) == -1)
 				return -1;
-			}
-			memset(solib, 0, sizeof(*solib));
-			strncpy(solib->name, optarg, PATH_MAX-1);
-			solib->name[PATH_MAX-1] = 0;
-			TAILQ_INSERT_TAIL(&solib_list, solib, next);
 			break;
 
 		/* long options */
-- 
2.4.3

[PATCH 2/5] eal: refactor plugin init from eal_parse_args() to a helper function

[Panu Matilainen]

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/linuxapp/eal/eal.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index cc66d9f..d8a53e4 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -548,6 +548,19 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
+static void
+eal_plugins_init(void)
+{
+	struct shared_driver *solib = NULL;
+
+	TAILQ_FOREACH(solib, &solib_list, next) {
+		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
+		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
+		if (solib->lib_handle == NULL)
+			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
+	}
+}
+
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -741,7 +754,6 @@ rte_eal_init(int argc, char **argv)
 	int i, fctret, ret;
 	pthread_t thread_id;
 	static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
-	struct shared_driver *solib = NULL;
 	const char *logid;
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 
@@ -837,12 +849,7 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
-	TAILQ_FOREACH(solib, &solib_list, next) {
-		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
-		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-		if (solib->lib_handle == NULL)
-			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
-	}
+	eal_plugins_init();
 
 	eal_thread_init_master(rte_config.master_lcore);
 
-- 
2.4.3

[PATCH 3/5] eal: move plugin loading to eal/common

[Panu Matilainen]

There's no good reason to limit plugins to Linux, make it available
on FreeBSD too.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/bsdapp/eal/eal.c            |  2 ++
 lib/librte_eal/common/eal_common_options.c | 52 +++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |  1 +
 lib/librte_eal/linuxapp/eal/eal.c          | 53 ------------------------------
 4 files changed, 55 insertions(+), 53 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 1b6f705..73dab89 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -543,6 +543,8 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
+	eal_plugins_init();
+
 	eal_thread_init_master(rte_config.master_lcore);
 
 	ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1f459ac..f8fc68a 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -39,6 +39,7 @@
 #include <limits.h>
 #include <errno.h>
 #include <getopt.h>
+#include <dlfcn.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>
@@ -93,6 +94,20 @@ eal_long_options[] = {
 	{0,                     0, NULL, 0                        }
 };
 
+TAILQ_HEAD(shared_driver_list, shared_driver);
+
+/* Definition for shared object drivers. */
+struct shared_driver {
+	TAILQ_ENTRY(shared_driver) next;
+
+	char    name[PATH_MAX];
+	void*   lib_handle;
+};
+
+/* List of external loadable drivers */
+static struct shared_driver_list solib_list =
+TAILQ_HEAD_INITIALIZER(solib_list);
+
 static int lcores_parsed;
 static int master_lcore_parsed;
 static int mem_parsed;
@@ -134,6 +149,37 @@ eal_reset_internal_config(struct internal_config *internal_cfg)
 	internal_cfg->create_uio_dev = 0;
 }
 
+static int
+eal_plugin_add(const char *path)
+{
+	struct shared_driver *solib;
+
+	solib = malloc(sizeof(*solib));
+	if (solib == NULL) {
+		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+		return -1;
+	}
+	memset(solib, 0, sizeof(*solib));
+	strncpy(solib->name, path, PATH_MAX-1);
+	solib->name[PATH_MAX-1] = 0;
+	TAILQ_INSERT_TAIL(&solib_list, solib, next);
+
+	return 0;
+}
+
+void
+eal_plugins_init(void)
+{
+	struct shared_driver *solib = NULL;
+
+	TAILQ_FOREACH(solib, &solib_list, next) {
+		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
+		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
+		if (solib->lib_handle == NULL)
+			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
+	}
+}
+
 /*
  * Parse the coremask given as argument (hexadecimal string) and fill
  * the global configuration (core role and core count) with the parsed
@@ -716,6 +762,11 @@ eal_parse_common_option(int opt, const char *optarg,
 		 * even if info or warning messages are disabled */
 		RTE_LOG(CRIT, EAL, "RTE Version: '%s'\n", rte_version());
 		break;
+	/* force loading of external driver */
+	case 'd':
+		if (eal_plugin_add(optarg) == -1)
+			return -1;
+		break;
 
 	/* long options */
 	case OPT_NO_HUGE_NUM:
@@ -902,6 +953,7 @@ eal_common_usage(void)
 	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
+	       "  -d LIB.so           Add driver (can be used multiple times)\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index f6714d9..1f96825 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -93,5 +93,6 @@ int eal_adjust_config(struct internal_config *internal_cfg);
 int eal_check_common_options(struct internal_config *internal_cfg);
 void eal_common_usage(void);
 enum rte_proc_type_t eal_proc_type_detect(void);
+void eal_plugins_init(void);
 
 #endif /* EAL_OPTIONS_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index d8a53e4..455243e 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -43,7 +43,6 @@
 #include <getopt.h>
 #include <sys/file.h>
 #include <fcntl.h>
-#include <dlfcn.h>
 #include <stddef.h>
 #include <errno.h>
 #include <limits.h>
@@ -90,20 +89,6 @@
 /* Allow the application to print its usage message too if set */
 static rte_usage_hook_t	rte_application_usage_hook = NULL;
 
-TAILQ_HEAD(shared_driver_list, shared_driver);
-
-/* Definition for shared object drivers. */
-struct shared_driver {
-	TAILQ_ENTRY(shared_driver) next;
-
-	char    name[PATH_MAX];
-	void*   lib_handle;
-};
-
-/* List of external loadable drivers */
-static struct shared_driver_list solib_list =
-TAILQ_HEAD_INITIALIZER(solib_list);
-
 /* early configuration structure, when memory config is not mmapped */
 static struct rte_mem_config early_mem_config;
 
@@ -350,7 +335,6 @@ eal_usage(const char *prgname)
 	printf("\nUsage: %s ", prgname);
 	eal_common_usage();
 	printf("EAL Linux options:\n"
-	       "  -d LIB.so           Add driver (can be used multiple times)\n"
 	       "  --"OPT_SOCKET_MEM"        Memory to allocate on sockets (comma separated values)\n"
 	       "  --"OPT_HUGE_DIR"          Directory where hugetlbfs is mounted\n"
 	       "  --"OPT_FILE_PREFIX"       Prefix for hugepage filenames\n"
@@ -530,37 +514,6 @@ eal_log_level_parse(int argc, char **argv)
 	optind = 0; /* reset getopt lib */
 }
 
-static int
-eal_plugin_add(const char *path)
-{
-	struct shared_driver *solib;
-
-	solib = malloc(sizeof(*solib));
-	if (solib == NULL) {
-		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
-		return -1;
-	}
-	memset(solib, 0, sizeof(*solib));
-	strncpy(solib->name, path, PATH_MAX-1);
-	solib->name[PATH_MAX-1] = 0;
-	TAILQ_INSERT_TAIL(&solib_list, solib, next);
-
-	return 0;
-}
-
-static void
-eal_plugins_init(void)
-{
-	struct shared_driver *solib = NULL;
-
-	TAILQ_FOREACH(solib, &solib_list, next) {
-		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
-		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-		if (solib->lib_handle == NULL)
-			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
-	}
-}
-
 /* Parse the argument given in the command line of the application */
 static int
 eal_parse_args(int argc, char **argv)
@@ -598,12 +551,6 @@ eal_parse_args(int argc, char **argv)
 			eal_usage(prgname);
 			exit(EXIT_SUCCESS);
 
-		/* force loading of external driver */
-		case 'd':
-			if (eal_plugin_add(optarg) == -1)
-				return -1;
-			break;
-
 		/* long options */
 		case OPT_XEN_DOM0_NUM:
 #ifdef RTE_LIBRTE_XEN_DOM0
-- 
2.4.3

[PATCH 4/5] eal: add an error code to plugin init for the next step

[Panu Matilainen]

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/bsdapp/eal/eal.c            | 3 ++-
 lib/librte_eal/common/eal_common_options.c | 3 ++-
 lib/librte_eal/common/eal_options.h        | 2 +-
 lib/librte_eal/linuxapp/eal/eal.c          | 3 ++-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 73dab89..f07a3c3 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -543,7 +543,8 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
-	eal_plugins_init();
+	if (eal_plugins_init() < 0)
+		rte_panic("Cannot init plugins\n");
 
 	eal_thread_init_master(rte_config.master_lcore);
 
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index f8fc68a..b542868 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -167,7 +167,7 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
-void
+int
 eal_plugins_init(void)
 {
 	struct shared_driver *solib = NULL;
@@ -178,6 +178,7 @@ eal_plugins_init(void)
 		if (solib->lib_handle == NULL)
 			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
 	}
+	return 0;
 }
 
 /*
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index 1f96825..e305fe8 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -93,6 +93,6 @@ int eal_adjust_config(struct internal_config *internal_cfg);
 int eal_check_common_options(struct internal_config *internal_cfg);
 void eal_common_usage(void);
 enum rte_proc_type_t eal_proc_type_detect(void);
-void eal_plugins_init(void);
+int eal_plugins_init(void);
 
 #endif /* EAL_OPTIONS_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 455243e..26285e3 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -796,7 +796,8 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
-	eal_plugins_init();
+	if (eal_plugins_init() < 0)
+		rte_panic("Cannot init plugins\n");
 
 	eal_thread_init_master(rte_config.master_lcore);
 
-- 
2.4.3

Re: [PATCH 4/5] eal: add an error code to plugin init for the next step

[Bruce Richardson]

On Fri, Oct 16, 2015 at 02:58:16PM +0300, Panu Matilainen wrote:
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c            | 3 ++-
>  lib/librte_eal/common/eal_common_options.c | 3 ++-
>  lib/librte_eal/common/eal_options.h        | 2 +-
>  lib/librte_eal/linuxapp/eal/eal.c          | 3 ++-
>  4 files changed, 7 insertions(+), 4 deletions(-)

Again, another minor nit, but couldn't this be done when refactoring in previous
patches, rather than needed a whole separate commit ?

/Bruce

Re: [PATCH 4/5] eal: add an error code to plugin init for the next step

[Panu Matilainen]

On 10/16/2015 03:59 PM, Bruce Richardson wrote:
> On Fri, Oct 16, 2015 at 02:58:16PM +0300, Panu Matilainen wrote:
>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
>> ---
>>   lib/librte_eal/bsdapp/eal/eal.c            | 3 ++-
>>   lib/librte_eal/common/eal_common_options.c | 3 ++-
>>   lib/librte_eal/common/eal_options.h        | 2 +-
>>   lib/librte_eal/linuxapp/eal/eal.c          | 3 ++-
>>   4 files changed, 7 insertions(+), 4 deletions(-)
>
> Again, another minor nit, but couldn't this be done when refactoring in previous
> patches, rather than needed a whole separate commit ?

Of course it'd be possible to do this earlier, I pondered about it too 
but then went with this because
a) otherwise I would've had to rework the earlier patches again
b) not knowing which way people prefer it, I might've had to rework it 
back to the original
c) didn't know we were saving commits
d) doing it like this maintains a certain symmetry to how stuff is 
introduced

... yes, its all rather academic :)

	- Panu -

Re: [PATCH 4/5] eal: add an error code to plugin init for the next step

[Panu Matilainen]

On 10/16/2015 04:14 PM, Panu Matilainen wrote:
> On 10/16/2015 03:59 PM, Bruce Richardson wrote:
>> On Fri, Oct 16, 2015 at 02:58:16PM +0300, Panu Matilainen wrote:
>>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
>>> ---
>>>   lib/librte_eal/bsdapp/eal/eal.c            | 3 ++-
>>>   lib/librte_eal/common/eal_common_options.c | 3 ++-
>>>   lib/librte_eal/common/eal_options.h        | 2 +-
>>>   lib/librte_eal/linuxapp/eal/eal.c          | 3 ++-
>>>   4 files changed, 7 insertions(+), 4 deletions(-)
>>
>> Again, another minor nit, but couldn't this be done when refactoring
>> in previous
>> patches, rather than needed a whole separate commit ?
>
> Of course it'd be possible to do this earlier, I pondered about it too
> but then went with this because
> a) otherwise I would've had to rework the earlier patches again
> b) not knowing which way people prefer it, I might've had to rework it
> back to the original
> c) didn't know we were saving commits
> d) doing it like this maintains a certain symmetry to how stuff is
> introduced

In other words: I spent many years working with a codebase where the 
policy was to never change code while moving it around otherwise. So 
yeah, matter of policy, taste and all, I'm clearly still learning where 
the fine line is in case of dpdk :)

The series can easily be shrunken into two logical steps if that's 
preferred:
1) move the plugin handling code to common
2) add the plugin directory support

	- Panu -

Re: [PATCH 4/5] eal: add an error code to plugin init for the next step

[Thomas Monjalon]

2015-10-16 16:38, Panu Matilainen:
> On 10/16/2015 04:14 PM, Panu Matilainen wrote:
> > On 10/16/2015 03:59 PM, Bruce Richardson wrote:
> >> On Fri, Oct 16, 2015 at 02:58:16PM +0300, Panu Matilainen wrote:
> >>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> >>> ---
> >>>   lib/librte_eal/bsdapp/eal/eal.c            | 3 ++-
> >>>   lib/librte_eal/common/eal_common_options.c | 3 ++-
> >>>   lib/librte_eal/common/eal_options.h        | 2 +-
> >>>   lib/librte_eal/linuxapp/eal/eal.c          | 3 ++-
> >>>   4 files changed, 7 insertions(+), 4 deletions(-)
> >>
> >> Again, another minor nit, but couldn't this be done when refactoring
> >> in previous
> >> patches, rather than needed a whole separate commit ?
> >
> > Of course it'd be possible to do this earlier, I pondered about it too
> > but then went with this because
> > a) otherwise I would've had to rework the earlier patches again
> > b) not knowing which way people prefer it, I might've had to rework it
> > back to the original
> > c) didn't know we were saving commits
> > d) doing it like this maintains a certain symmetry to how stuff is
> > introduced
> 
> In other words: I spent many years working with a codebase where the 
> policy was to never change code while moving it around otherwise. So 
> yeah, matter of policy, taste and all, I'm clearly still learning where 
> the fine line is in case of dpdk :)
> 
> The series can easily be shrunken into two logical steps if that's 
> preferred:
> 1) move the plugin handling code to common
> 2) add the plugin directory support

Yes, looks good. Thanks

[PATCH 5/5] eal: add support for driver directory concept

[Panu Matilainen]

Add a new EAL option -D for loading all drivers from a given directory.
Additionally a default driver directory can be set in build-time
configuration, in which case it will be always be used when EAL is
initialized (but can be overridden or disabled with -D).

This simplifies usage in shared library configuration significantly over
manually loading individual drivers with -d, and allows distros to
establish a drop-in driver directory for seamless integration
with 3rd party drivers etc.

Suggested-by: homas Monjalon <thomas.monjalon@6wind.com>
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 config/common_bsdapp                       |  3 ++
 config/common_linuxapp                     |  3 ++
 lib/librte_eal/common/eal_common_options.c | 50 ++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index b37dcf4..13bccf4 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -108,6 +108,9 @@ CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
 CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # FreeBSD contiguous memory driver settings
 #
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0de43d5..a0d8cd2 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -111,6 +111,9 @@ CONFIG_RTE_EAL_IGB_UIO=y
 CONFIG_RTE_EAL_VFIO=y
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # Special configurations in PCI Config Space for high performance
 #
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index b542868..c2aca91 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -40,6 +40,8 @@
 #include <errno.h>
 #include <getopt.h>
 #include <dlfcn.h>
+#include <sys/types.h>
+#include <dirent.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>
@@ -59,6 +61,7 @@ eal_short_options[] =
 	"b:" /* pci-blacklist */
 	"c:" /* coremask */
 	"d:" /* driver */
+	"D:" /* driver directory */
 	"h"  /* help */
 	"l:" /* corelist */
 	"m:" /* memory size */
@@ -108,6 +111,9 @@ struct shared_driver {
 static struct shared_driver_list solib_list =
 TAILQ_HEAD_INITIALIZER(solib_list);
 
+/* Path of external loadable drivers */
+static const char *solib_dir = RTE_EAL_PMD_PATH;
+
 static int lcores_parsed;
 static int master_lcore_parsed;
 static int mem_parsed;
@@ -167,11 +173,50 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
+static int
+eal_plugindir_init(const char *path)
+{
+	DIR *d = NULL;
+	struct dirent *dent = NULL;
+	char sopath[PATH_MAX];
+
+	if (path == NULL || *path == '\0')
+		return 0;
+
+	d = opendir(path);
+	if (d == NULL) {
+		RTE_LOG(ERR, EAL, "failed to open directory %s: %s\n",
+			path, strerror(errno));
+		return -1;
+	}
+
+	while ((dent = readdir(d)) != NULL) {
+		if (dent->d_type != DT_REG && dent->d_type != DT_LNK)
+		continue;
+
+		snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name);
+		sopath[PATH_MAX-1] = 0;
+
+		if (eal_plugin_add(sopath) == -1)
+		    break;
+	}
+
+	closedir(d);
+	/* XXX this ignores failures from readdir() itself */
+	return (dent == NULL) ? 0 : -1;
+}
+
 int
 eal_plugins_init(void)
 {
 	struct shared_driver *solib = NULL;
 
+	if (eal_plugindir_init(solib_dir) == -1) {
+		RTE_LOG(ERR, EAL, "Cannot init plugin directory %s\n",
+			solib_dir);
+		return -1;
+	}
+
 	TAILQ_FOREACH(solib, &solib_list, next) {
 		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
 		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
@@ -768,6 +813,10 @@ eal_parse_common_option(int opt, const char *optarg,
 		if (eal_plugin_add(optarg) == -1)
 			return -1;
 		break;
+	/* set external driver directory */
+	case 'D':
+		solib_dir = optarg;
+		break;
 
 	/* long options */
 	case OPT_NO_HUGE_NUM:
@@ -955,6 +1004,7 @@ eal_common_usage(void)
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
 	       "  -d LIB.so           Add driver (can be used multiple times)\n"
+	       "  -D DIRECTORY        Add driver directory)\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
-- 
2.4.3

Re: [PATCH 1/5] eal: refactor plugin list append from eal_parse_args() to a helper function

[Bruce Richardson]

On Fri, Oct 16, 2015 at 02:58:13PM +0300, Panu Matilainen wrote:
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 33e1067..cc66d9f 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -530,6 +530,24 @@ eal_log_level_parse(int argc, char **argv)
>  	optind = 0; /* reset getopt lib */
>  }
>  
> +static int
> +eal_plugin_add(const char *path)
> +{
> +	struct shared_driver *solib;
> +
> +	solib = malloc(sizeof(*solib));
> +	if (solib == NULL) {
> +		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
> +		return -1;
> +	}
> +	memset(solib, 0, sizeof(*solib));
> +	strncpy(solib->name, path, PATH_MAX-1);
> +	solib->name[PATH_MAX-1] = 0;

I always prefer a one-line snprintf to the above two-line code. :-)

/Bruce

Re: [PATCH 1/5] eal: refactor plugin list append from eal_parse_args() to a helper function

[Panu Matilainen]

On 10/16/2015 03:57 PM, Bruce Richardson wrote:
> On Fri, Oct 16, 2015 at 02:58:13PM +0300, Panu Matilainen wrote:
>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++---------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
>> index 33e1067..cc66d9f 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal.c
>> @@ -530,6 +530,24 @@ eal_log_level_parse(int argc, char **argv)
>>   	optind = 0; /* reset getopt lib */
>>   }
>>
>> +static int
>> +eal_plugin_add(const char *path)
>> +{
>> +	struct shared_driver *solib;
>> +
>> +	solib = malloc(sizeof(*solib));
>> +	if (solib == NULL) {
>> +		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
>> +		return -1;
>> +	}
>> +	memset(solib, 0, sizeof(*solib));
>> +	strncpy(solib->name, path, PATH_MAX-1);
>> +	solib->name[PATH_MAX-1] = 0;
>
> I always prefer a one-line snprintf to the above two-line code. :-)

Me too (or asprintf, depending on situation), but the point of this 
patch is to move around existing code without changing it.
Certainly I can change it to sprintf if that's preferred.

	- Panu -

[PATCH 0/2 v3] Add support for driver directories

[Panu Matilainen]

This mini-series adds support for driver directory concept
based on idea by Thomas Monjalon back in February:
http://dpdk.org/ml/archives/dev/2015-February/013285.html

In the process FreeBSD also gains plugin support (but untested).

v3: merge the first commits

v2: move code to eal/common, add bsd support

Panu Matilainen (2):
  eal: move plugin loading to eal/common
  eal: add support for driver directory concept

 config/common_bsdapp                       |   3 +
 config/common_linuxapp                     |   3 +
 lib/librte_eal/bsdapp/eal/eal.c            |   3 +
 lib/librte_eal/common/eal_common_options.c | 103 +++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |   1 +
 lib/librte_eal/linuxapp/eal/eal.c          |  39 +----------
 6 files changed, 115 insertions(+), 37 deletions(-)

-- 
2.4.3

[PATCH 1/2] eal: move plugin loading to eal/common

[Panu Matilainen]

There's no good reason to limit plugins to Linux, make it available
on FreeBSD too. Refactor the plugin code from Linux EAL to common
helper functions, also check for potential errors during initialization.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/bsdapp/eal/eal.c            |  3 ++
 lib/librte_eal/common/eal_common_options.c | 53 ++++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |  1 +
 lib/librte_eal/linuxapp/eal/eal.c          | 39 ++--------------------
 4 files changed, 59 insertions(+), 37 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 1b6f705..f07a3c3 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
+	if (eal_plugins_init() < 0)
+		rte_panic("Cannot init plugins\n");
+
 	eal_thread_init_master(rte_config.master_lcore);
 
 	ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1f459ac..b542868 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -39,6 +39,7 @@
 #include <limits.h>
 #include <errno.h>
 #include <getopt.h>
+#include <dlfcn.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>
@@ -93,6 +94,20 @@ eal_long_options[] = {
 	{0,                     0, NULL, 0                        }
 };
 
+TAILQ_HEAD(shared_driver_list, shared_driver);
+
+/* Definition for shared object drivers. */
+struct shared_driver {
+	TAILQ_ENTRY(shared_driver) next;
+
+	char    name[PATH_MAX];
+	void*   lib_handle;
+};
+
+/* List of external loadable drivers */
+static struct shared_driver_list solib_list =
+TAILQ_HEAD_INITIALIZER(solib_list);
+
 static int lcores_parsed;
 static int master_lcore_parsed;
 static int mem_parsed;
@@ -134,6 +149,38 @@ eal_reset_internal_config(struct internal_config *internal_cfg)
 	internal_cfg->create_uio_dev = 0;
 }
 
+static int
+eal_plugin_add(const char *path)
+{
+	struct shared_driver *solib;
+
+	solib = malloc(sizeof(*solib));
+	if (solib == NULL) {
+		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+		return -1;
+	}
+	memset(solib, 0, sizeof(*solib));
+	strncpy(solib->name, path, PATH_MAX-1);
+	solib->name[PATH_MAX-1] = 0;
+	TAILQ_INSERT_TAIL(&solib_list, solib, next);
+
+	return 0;
+}
+
+int
+eal_plugins_init(void)
+{
+	struct shared_driver *solib = NULL;
+
+	TAILQ_FOREACH(solib, &solib_list, next) {
+		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
+		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
+		if (solib->lib_handle == NULL)
+			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
+	}
+	return 0;
+}
+
 /*
  * Parse the coremask given as argument (hexadecimal string) and fill
  * the global configuration (core role and core count) with the parsed
@@ -716,6 +763,11 @@ eal_parse_common_option(int opt, const char *optarg,
 		 * even if info or warning messages are disabled */
 		RTE_LOG(CRIT, EAL, "RTE Version: '%s'\n", rte_version());
 		break;
+	/* force loading of external driver */
+	case 'd':
+		if (eal_plugin_add(optarg) == -1)
+			return -1;
+		break;
 
 	/* long options */
 	case OPT_NO_HUGE_NUM:
@@ -902,6 +954,7 @@ eal_common_usage(void)
 	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
+	       "  -d LIB.so           Add driver (can be used multiple times)\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index f6714d9..e305fe8 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -93,5 +93,6 @@ int eal_adjust_config(struct internal_config *internal_cfg);
 int eal_check_common_options(struct internal_config *internal_cfg);
 void eal_common_usage(void);
 enum rte_proc_type_t eal_proc_type_detect(void);
+int eal_plugins_init(void);
 
 #endif /* EAL_OPTIONS_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index e0ad1d7..c09176f 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -43,7 +43,6 @@
 #include <getopt.h>
 #include <sys/file.h>
 #include <fcntl.h>
-#include <dlfcn.h>
 #include <stddef.h>
 #include <errno.h>
 #include <limits.h>
@@ -90,20 +89,6 @@
 /* Allow the application to print its usage message too if set */
 static rte_usage_hook_t	rte_application_usage_hook = NULL;
 
-TAILQ_HEAD(shared_driver_list, shared_driver);
-
-/* Definition for shared object drivers. */
-struct shared_driver {
-	TAILQ_ENTRY(shared_driver) next;
-
-	char    name[PATH_MAX];
-	void*   lib_handle;
-};
-
-/* List of external loadable drivers */
-static struct shared_driver_list solib_list =
-TAILQ_HEAD_INITIALIZER(solib_list);
-
 /* early configuration structure, when memory config is not mmapped */
 static struct rte_mem_config early_mem_config;
 
@@ -350,7 +335,6 @@ eal_usage(const char *prgname)
 	printf("\nUsage: %s ", prgname);
 	eal_common_usage();
 	printf("EAL Linux options:\n"
-	       "  -d LIB.so           Add driver (can be used multiple times)\n"
 	       "  --"OPT_SOCKET_MEM"        Memory to allocate on sockets (comma separated values)\n"
 	       "  --"OPT_HUGE_DIR"          Directory where hugetlbfs is mounted\n"
 	       "  --"OPT_FILE_PREFIX"       Prefix for hugepage filenames\n"
@@ -538,7 +522,6 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	struct shared_driver *solib;
 
 	argvopt = argv;
 
@@ -568,19 +551,6 @@ eal_parse_args(int argc, char **argv)
 			eal_usage(prgname);
 			exit(EXIT_SUCCESS);
 
-		/* force loading of external driver */
-		case 'd':
-			solib = malloc(sizeof(*solib));
-			if (solib == NULL) {
-				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
-				return -1;
-			}
-			memset(solib, 0, sizeof(*solib));
-			strncpy(solib->name, optarg, PATH_MAX-1);
-			solib->name[PATH_MAX-1] = 0;
-			TAILQ_INSERT_TAIL(&solib_list, solib, next);
-			break;
-
 		/* long options */
 		case OPT_XEN_DOM0_NUM:
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -731,7 +701,6 @@ rte_eal_init(int argc, char **argv)
 	int i, fctret, ret;
 	pthread_t thread_id;
 	static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
-	struct shared_driver *solib = NULL;
 	const char *logid;
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 
@@ -824,12 +793,8 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
-	TAILQ_FOREACH(solib, &solib_list, next) {
-		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
-		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-		if (solib->lib_handle == NULL)
-			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
-	}
+	if (eal_plugins_init() < 0)
+		rte_panic("Cannot init plugins\n");
 
 	eal_thread_init_master(rte_config.master_lcore);
 
-- 
2.4.3

Re: [PATCH 1/2] eal: move plugin loading to eal/common

[David Marchand]

On Wed, Oct 21, 2015 at 10:29 AM, Panu Matilainen <pmatilai@redhat.com>
wrote:

> There's no good reason to limit plugins to Linux, make it available
> on FreeBSD too. Refactor the plugin code from Linux EAL to common
> helper functions, also check for potential errors during initialization.
>

Not sure about this "potential errors".


>
> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c            |  3 ++
>  lib/librte_eal/common/eal_common_options.c | 53
> ++++++++++++++++++++++++++++++
>  lib/librte_eal/common/eal_options.h        |  1 +
>  lib/librte_eal/linuxapp/eal/eal.c          | 39 ++--------------------
>  4 files changed, 59 insertions(+), 37 deletions(-)
>
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
> b/lib/librte_eal/bsdapp/eal/eal.c
> index 1b6f705..f07a3c3 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
>
>         rte_eal_mcfg_complete();
>
> +       if (eal_plugins_init() < 0)
> +               rte_panic("Cannot init plugins\n");
> +
>

Why testing for error when this cannot happen (see below) ?


>         eal_thread_init_master(rte_config.master_lcore);
>
>         ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
> diff --git a/lib/librte_eal/common/eal_common_options.c
> b/lib/librte_eal/common/eal_common_options.c
> index 1f459ac..b542868 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
>

[snip]

+
> +int
> +eal_plugins_init(void)
> +{
> +       struct shared_driver *solib = NULL;
> +
> +       TAILQ_FOREACH(solib, &solib_list, next) {
> +               RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
> +               solib->lib_handle = dlopen(solib->name, RTLD_NOW);
> +               if (solib->lib_handle == NULL)
> +                       RTE_LOG(WARNING, EAL, "%s\n", dlerror());
> +       }
> +       return 0;
> +}


I can't see a non 0 return here.

Btw, returning an error here would change current behavior of dpdk loading
drivers.
Not sure we want this as people may rely on this loading not failing.


-- 
David Marchand

Re: [PATCH 1/2] eal: move plugin loading to eal/common

[Panu Matilainen]

On 10/21/2015 01:15 PM, David Marchand wrote:
> On Wed, Oct 21, 2015 at 10:29 AM, Panu Matilainen <pmatilai@redhat.com>
> wrote:
>
>> There's no good reason to limit plugins to Linux, make it available
>> on FreeBSD too. Refactor the plugin code from Linux EAL to common
>> helper functions, also check for potential errors during initialization.
>>
>
> Not sure about this "potential errors".
>
>
>>
>> Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
>> ---
>>   lib/librte_eal/bsdapp/eal/eal.c            |  3 ++
>>   lib/librte_eal/common/eal_common_options.c | 53
>> ++++++++++++++++++++++++++++++
>>   lib/librte_eal/common/eal_options.h        |  1 +
>>   lib/librte_eal/linuxapp/eal/eal.c          | 39 ++--------------------
>>   4 files changed, 59 insertions(+), 37 deletions(-)
>>
>> diff --git a/lib/librte_eal/bsdapp/eal/eal.c
>> b/lib/librte_eal/bsdapp/eal/eal.c
>> index 1b6f705..f07a3c3 100644
>> --- a/lib/librte_eal/bsdapp/eal/eal.c
>> +++ b/lib/librte_eal/bsdapp/eal/eal.c
>> @@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
>>
>>          rte_eal_mcfg_complete();
>>
>> +       if (eal_plugins_init() < 0)
>> +               rte_panic("Cannot init plugins\n");
>> +
>>
>
> Why testing for error when this cannot happen (see below) ?

This is just a preparation for the next patch which will add a 
possibility of failure. Its done here to get the new "infrastructure" in 
place in one commit, which seemed to be the preferred option by others.

>>          eal_thread_init_master(rte_config.master_lcore);
>>
>>          ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
>> diff --git a/lib/librte_eal/common/eal_common_options.c
>> b/lib/librte_eal/common/eal_common_options.c
>> index 1f459ac..b542868 100644
>> --- a/lib/librte_eal/common/eal_common_options.c
>> +++ b/lib/librte_eal/common/eal_common_options.c
>>
>
> [snip]
>
> +
>> +int
>> +eal_plugins_init(void)
>> +{
>> +       struct shared_driver *solib = NULL;
>> +
>> +       TAILQ_FOREACH(solib, &solib_list, next) {
>> +               RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
>> +               solib->lib_handle = dlopen(solib->name, RTLD_NOW);
>> +               if (solib->lib_handle == NULL)
>> +                       RTE_LOG(WARNING, EAL, "%s\n", dlerror());
>> +       }
>> +       return 0;
>> +}
>
>
> I can't see a non 0 return here.

Yes, there is none, see above.

> Btw, returning an error here would change current behavior of dpdk loading
> drivers.
> Not sure we want this as people may rely on this loading not failing.

Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is 
actually fairly questionable behavior. Why would you load drivers with 
-d if you dont care about them getting loaded? Well, maybe to handle an 
"everything" case but that's much better handled with the driver 
directory thing.

So actually the current patches make things a bit inconsistent, why 
should driver directories cause a failure if individual drivers do not? 
The question is, which behavior is the one people want: I personally 
would rather make -dgiddy.goo fail rather than just warn and chug away 
but its not exactly a deal-breaker for me.

	- Panu -



>

Re: [PATCH 1/2] eal: move plugin loading to eal/common

[David Marchand]

On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:
>
> Btw, returning an error here would change current behavior of dpdk loading
>> drivers.
>> Not sure we want this as people may rely on this loading not failing.
>>
>
> Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
> actually fairly questionable behavior. Why would you load drivers with -d
> if you dont care about them getting loaded? Well, maybe to handle an
> "everything" case but that's much better handled with the driver directory
> thing.
>
> So actually the current patches make things a bit inconsistent, why should
> driver directories cause a failure if individual drivers do not? The
> question is, which behavior is the one people want: I personally would
> rather make -dgiddy.goo fail rather than just warn and chug away but its
> not exactly a deal-breaker for me.
>

Neither to me.
I agree on the principle of failing when passing wrong stuff, it is saner.
I just want to make sure nobody complains about this change later.

Thomas ? Bruce ?


-- 
David Marchand

Re: [PATCH 1/2] eal: move plugin loading to eal/common

[Bruce Richardson]

On Wed, Oct 21, 2015 at 01:09:24PM +0200, David Marchand wrote:
> On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
> wrote:
> >
> > Btw, returning an error here would change current behavior of dpdk loading
> >> drivers.
> >> Not sure we want this as people may rely on this loading not failing.
> >>
> >
> > Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
> > actually fairly questionable behavior. Why would you load drivers with -d
> > if you dont care about them getting loaded? Well, maybe to handle an
> > "everything" case but that's much better handled with the driver directory
> > thing.
> >
> > So actually the current patches make things a bit inconsistent, why should
> > driver directories cause a failure if individual drivers do not? The
> > question is, which behavior is the one people want: I personally would
> > rather make -dgiddy.goo fail rather than just warn and chug away but its
> > not exactly a deal-breaker for me.
> >
> 
> Neither to me.
> I agree on the principle of failing when passing wrong stuff, it is saner.
> I just want to make sure nobody complains about this change later.
> 
> Thomas ? Bruce ?
> 

Error early rather than later. If the -d flag doesn't work crash then, rather
than leaving people having to scroll-back to find why their NIC isn't working.

/Bruce

Re: [PATCH 1/2] eal: move plugin loading to eal/common

[Thomas Monjalon]

2015-10-21 12:15, Bruce Richardson:
> On Wed, Oct 21, 2015 at 01:09:24PM +0200, David Marchand wrote:
> > On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
> > wrote:
> > >
> > > Btw, returning an error here would change current behavior of dpdk loading
> > >> drivers.
> > >> Not sure we want this as people may rely on this loading not failing.
> > >>
> > >
> > > Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
> > > actually fairly questionable behavior. Why would you load drivers with -d
> > > if you dont care about them getting loaded? Well, maybe to handle an
> > > "everything" case but that's much better handled with the driver directory
> > > thing.
> > >
> > > So actually the current patches make things a bit inconsistent, why should
> > > driver directories cause a failure if individual drivers do not? The
> > > question is, which behavior is the one people want: I personally would
> > > rather make -dgiddy.goo fail rather than just warn and chug away but its
> > > not exactly a deal-breaker for me.
> > >
> > 
> > Neither to me.
> > I agree on the principle of failing when passing wrong stuff, it is saner.
> > I just want to make sure nobody complains about this change later.
> > 
> > Thomas ? Bruce ?
> 
> Error early rather than later. If the -d flag doesn't work crash then, rather
> than leaving people having to scroll-back to find why their NIC isn't working.

Yes, no reason to ignore errors.

Re: [PATCH 1/2] eal: move plugin loading to eal/common

[Panu Matilainen]

On 10/21/2015 02:53 PM, Thomas Monjalon wrote:
> 2015-10-21 12:15, Bruce Richardson:
>> On Wed, Oct 21, 2015 at 01:09:24PM +0200, David Marchand wrote:
>>> On Wed, Oct 21, 2015 at 12:54 PM, Panu Matilainen <pmatilai@redhat.com>
>>> wrote:
>>>>
>>>> Btw, returning an error here would change current behavior of dpdk loading
>>>>> drivers.
>>>>> Not sure we want this as people may rely on this loading not failing.
>>>>>
>>>>
>>>> Yeah, dpdk currently doesn't fail if you pass garbage to -d, which is
>>>> actually fairly questionable behavior. Why would you load drivers with -d
>>>> if you dont care about them getting loaded? Well, maybe to handle an
>>>> "everything" case but that's much better handled with the driver directory
>>>> thing.
>>>>
>>>> So actually the current patches make things a bit inconsistent, why should
>>>> driver directories cause a failure if individual drivers do not? The
>>>> question is, which behavior is the one people want: I personally would
>>>> rather make -dgiddy.goo fail rather than just warn and chug away but its
>>>> not exactly a deal-breaker for me.
>>>>
>>>
>>> Neither to me.
>>> I agree on the principle of failing when passing wrong stuff, it is saner.
>>> I just want to make sure nobody complains about this change later.
>>>
>>> Thomas ? Bruce ?
>>
>> Error early rather than later. If the -d flag doesn't work crash then, rather
>> than leaving people having to scroll-back to find why their NIC isn't working.
>
> Yes, no reason to ignore errors.

Okay so we all vigorously agree on this :) Good then, I'll fix the error 
behavior too in the next version.

	 - Panu -

[PATCH 2/2] eal: add support for driver directory concept

[Panu Matilainen]

Add a new EAL option -D for loading all drivers from a given directory.
Additionally a default driver directory can be set in build-time
configuration, in which case it will be always be used when EAL is
initialized (but can be overridden or disabled with -D).

This simplifies usage in shared library configuration significantly over
manually loading individual drivers with -d, and allows distros to
establish a drop-in driver directory for seamless integration
with 3rd party drivers etc.

Suggested-by: Thomas Monjalon <thomas.monjalon@6wind.com>
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 config/common_bsdapp                       |  3 ++
 config/common_linuxapp                     |  3 ++
 lib/librte_eal/common/eal_common_options.c | 50 ++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index b37dcf4..13bccf4 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -108,6 +108,9 @@ CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
 CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # FreeBSD contiguous memory driver settings
 #
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0de43d5..a0d8cd2 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -111,6 +111,9 @@ CONFIG_RTE_EAL_IGB_UIO=y
 CONFIG_RTE_EAL_VFIO=y
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # Special configurations in PCI Config Space for high performance
 #
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index b542868..c2aca91 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -40,6 +40,8 @@
 #include <errno.h>
 #include <getopt.h>
 #include <dlfcn.h>
+#include <sys/types.h>
+#include <dirent.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>
@@ -59,6 +61,7 @@ eal_short_options[] =
 	"b:" /* pci-blacklist */
 	"c:" /* coremask */
 	"d:" /* driver */
+	"D:" /* driver directory */
 	"h"  /* help */
 	"l:" /* corelist */
 	"m:" /* memory size */
@@ -108,6 +111,9 @@ struct shared_driver {
 static struct shared_driver_list solib_list =
 TAILQ_HEAD_INITIALIZER(solib_list);
 
+/* Path of external loadable drivers */
+static const char *solib_dir = RTE_EAL_PMD_PATH;
+
 static int lcores_parsed;
 static int master_lcore_parsed;
 static int mem_parsed;
@@ -167,11 +173,50 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
+static int
+eal_plugindir_init(const char *path)
+{
+	DIR *d = NULL;
+	struct dirent *dent = NULL;
+	char sopath[PATH_MAX];
+
+	if (path == NULL || *path == '\0')
+		return 0;
+
+	d = opendir(path);
+	if (d == NULL) {
+		RTE_LOG(ERR, EAL, "failed to open directory %s: %s\n",
+			path, strerror(errno));
+		return -1;
+	}
+
+	while ((dent = readdir(d)) != NULL) {
+		if (dent->d_type != DT_REG && dent->d_type != DT_LNK)
+		continue;
+
+		snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name);
+		sopath[PATH_MAX-1] = 0;
+
+		if (eal_plugin_add(sopath) == -1)
+		    break;
+	}
+
+	closedir(d);
+	/* XXX this ignores failures from readdir() itself */
+	return (dent == NULL) ? 0 : -1;
+}
+
 int
 eal_plugins_init(void)
 {
 	struct shared_driver *solib = NULL;
 
+	if (eal_plugindir_init(solib_dir) == -1) {
+		RTE_LOG(ERR, EAL, "Cannot init plugin directory %s\n",
+			solib_dir);
+		return -1;
+	}
+
 	TAILQ_FOREACH(solib, &solib_list, next) {
 		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
 		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
@@ -768,6 +813,10 @@ eal_parse_common_option(int opt, const char *optarg,
 		if (eal_plugin_add(optarg) == -1)
 			return -1;
 		break;
+	/* set external driver directory */
+	case 'D':
+		solib_dir = optarg;
+		break;
 
 	/* long options */
 	case OPT_NO_HUGE_NUM:
@@ -955,6 +1004,7 @@ eal_common_usage(void)
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
 	       "  -d LIB.so           Add driver (can be used multiple times)\n"
+	       "  -D DIRECTORY        Add driver directory)\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
-- 
2.4.3

Re: [PATCH 2/2] eal: add support for driver directory concept

[Thomas Monjalon]

2015-10-21 11:29, Panu Matilainen:
> Add a new EAL option -D for loading all drivers from a given directory.
> Additionally a default driver directory can be set in build-time
> configuration, in which case it will be always be used when EAL is
> initialized (but can be overridden or disabled with -D).
[...]
> @@ -955,6 +1004,7 @@ eal_common_usage(void)
>  	       "  --"OPT_SYSLOG"            Set syslog facility\n"
>  	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
>  	       "  -d LIB.so           Add driver (can be used multiple times)\n"
> +	       "  -D DIRECTORY        Add driver directory)\n"

Another idea: instead of adding a new option, why not make -d able to deal
with directories?

Re: [PATCH 2/2] eal: add support for driver directory concept

[Panu Matilainen]

On 10/21/2015 11:44 AM, Thomas Monjalon wrote:
> 2015-10-21 11:29, Panu Matilainen:
>> Add a new EAL option -D for loading all drivers from a given directory.
>> Additionally a default driver directory can be set in build-time
>> configuration, in which case it will be always be used when EAL is
>> initialized (but can be overridden or disabled with -D).
> [...]
>> @@ -955,6 +1004,7 @@ eal_common_usage(void)
>>   	       "  --"OPT_SYSLOG"            Set syslog facility\n"
>>   	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
>>   	       "  -d LIB.so           Add driver (can be used multiple times)\n"
>> +	       "  -D DIRECTORY        Add driver directory)\n"
>
> Another idea: instead of adding a new option, why not make -d able to deal
> with directories?
>

If that's what you want, sure. Just wishing you'd come up with this idea 
a bit earlier :)

	- Panu -

[PATCH v4 0/2] Add support for driver directories

[Panu Matilainen]

This mini-series adds support for driver directory concept
based on idea by Thomas Monjalon back in February:
http://dpdk.org/ml/archives/dev/2015-February/013285.html

In the process FreeBSD also gains plugin support (but untested).

v4: - introduce error-early behavior for invalid plugin paths
    - support directories via the existing -d option instead of adding new

v3: - merge the first commits

v2: - move code to eal/common
    - add bsd support

Panu Matilainen (2):
  eal: move plugin loading to eal/common
  eal: add support for driver directory concept

 config/common_bsdapp                       |   3 +
 config/common_linuxapp                     |   3 +
 lib/librte_eal/bsdapp/eal/eal.c            |   3 +
 lib/librte_eal/common/eal_common_options.c | 119 +++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |   1 +
 lib/librte_eal/linuxapp/eal/eal.c          |  40 +---------
 6 files changed, 131 insertions(+), 38 deletions(-)

-- 
2.5.0

Re: [PATCH v4 0/2] Add support for driver directories

[David Marchand]

Hello,

On Tue, Nov 10, 2015 at 3:28 PM, Panu Matilainen <pmatilai@redhat.com>
wrote:

> This mini-series adds support for driver directory concept
> based on idea by Thomas Monjalon back in February:
> http://dpdk.org/ml/archives/dev/2015-February/013285.html
>
> In the process FreeBSD also gains plugin support (but untested).
>
> v4: - introduce error-early behavior for invalid plugin paths
>     - support directories via the existing -d option instead of adding new
>
> v3: - merge the first commits
>
> v2: - move code to eal/common
>     - add bsd support
>
> Panu Matilainen (2):
>   eal: move plugin loading to eal/common
>   eal: add support for driver directory concept


checkpatch complains for some indent problem (Thomas, can you fix this ?),
but the rest looks good to me.

Acked-by: David Marchand <david.marchand@6wind.com>

Thanks Panu.

-- 
David Marchand

Re: [PATCH v4 0/2] Add support for driver directories

[Thomas Monjalon]

> > This mini-series adds support for driver directory concept
> > based on idea by Thomas Monjalon back in February:
> > http://dpdk.org/ml/archives/dev/2015-February/013285.html
> >
> > In the process FreeBSD also gains plugin support (but untested).
> >
> > v4: - introduce error-early behavior for invalid plugin paths
> >     - support directories via the existing -d option instead of adding new
> >
> > v3: - merge the first commits
> >
> > v2: - move code to eal/common
> >     - add bsd support
> >
> > Panu Matilainen (2):
> >   eal: move plugin loading to eal/common
> >   eal: add support for driver directory concept
> 
> 
> checkpatch complains for some indent problem (Thomas, can you fix this ?),
> but the rest looks good to me.
> 
> Acked-by: David Marchand <david.marchand@6wind.com>
> 
> Thanks Panu.

Applied, thanks

Re: [PATCH v4 0/2] Add support for driver directories

[Stephen Hemminger]

On Thu, 12 Nov 2015 16:52:32 +0100
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> > > This mini-series adds support for driver directory concept
> > > based on idea by Thomas Monjalon back in February:
> > > http://dpdk.org/ml/archives/dev/2015-February/013285.html
> > >
> > > In the process FreeBSD also gains plugin support (but untested).
> > >
> > > v4: - introduce error-early behavior for invalid plugin paths
> > >     - support directories via the existing -d option instead of adding new
> > >
> > > v3: - merge the first commits
> > >
> > > v2: - move code to eal/common
> > >     - add bsd support
> > >
> > > Panu Matilainen (2):
> > >   eal: move plugin loading to eal/common
> > >   eal: add support for driver directory concept
> > 
> > 
> > checkpatch complains for some indent problem (Thomas, can you fix this ?),
> > but the rest looks good to me.
> > 
> > Acked-by: David Marchand <david.marchand@6wind.com>
> > 
> > Thanks Panu.
> 
> Applied, thanks

This patch introduces a new issue reported by Coverity.

The root cause of the problem is that you are checking that it s a directory first with stat
then calling dlopen(). I malicious entity could get between the stat and the dlopen.

In this case the desire to handle both file name and directory is getting in the way.
It really should just only take a directory now, or have two different config options
in a method similar to other subsystems (look at /etc/xxx vs /etc/xxx.d as standard practice).

________________________________________________________________________________________________________
*** CID 120151:  Security best practices violations  (TOCTOU)
/lib/librte_eal/common/eal_common_options.c: 232 in eal_plugins_init()
226     					solib->name);
227     				return -1;
228     			}
229     		} else {
230     			RTE_LOG(DEBUG, EAL, "open shared lib %s\n",
231     				solib->name);
>>>     CID 120151:  Security best practices violations  (TOCTOU)
>>>     Calling function "dlopen" that uses "solib->name" after a check function. This can cause a time-of-check, time-of-use race condition.  
232     			solib->lib_handle = dlopen(solib->name, RTLD_NOW);
233     			if (solib->lib_handle == NULL) {
234     				RTE_LOG(ERR, EAL, "%s\n", dlerror());
235     				return -1;
236     			}
237     		}

Re: [PATCH v4 0/2] Add support for driver directories

[Thomas Monjalon]

2015-12-02 18:07, Stephen Hemminger:
> On Thu, 12 Nov 2015 16:52:32 +0100
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> > > > This mini-series adds support for driver directory concept
> > > > based on idea by Thomas Monjalon back in February:
> > > > http://dpdk.org/ml/archives/dev/2015-February/013285.html
> > > >
> > > > In the process FreeBSD also gains plugin support (but untested).
> > > >
> > > > v4: - introduce error-early behavior for invalid plugin paths
> > > >     - support directories via the existing -d option instead of adding new
> > > >
> > > > v3: - merge the first commits
> > > >
> > > > v2: - move code to eal/common
> > > >     - add bsd support
> > > >
> > > > Panu Matilainen (2):
> > > >   eal: move plugin loading to eal/common
> > > >   eal: add support for driver directory concept
> > > 
> > > 
> > > checkpatch complains for some indent problem (Thomas, can you fix this ?),
> > > but the rest looks good to me.
> > > 
> > > Acked-by: David Marchand <david.marchand@6wind.com>
> > > 
> > > Thanks Panu.
> > 
> > Applied, thanks
> 
> This patch introduces a new issue reported by Coverity.
> 
> The root cause of the problem is that you are checking that it s a directory first with stat
> then calling dlopen(). I malicious entity could get between the stat and the dlopen.

I think it is a false positive.
The aim of loading every files in the directory is out of a security scope IMHO.

Re: [PATCH v4 0/2] Add support for driver directories

[Panu Matilainen]

On 12/03/2015 04:26 AM, Thomas Monjalon wrote:
> 2015-12-02 18:07, Stephen Hemminger:
>> On Thu, 12 Nov 2015 16:52:32 +0100
>> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
>>
>>>>> This mini-series adds support for driver directory concept
>>>>> based on idea by Thomas Monjalon back in February:
>>>>> http://dpdk.org/ml/archives/dev/2015-February/013285.html
>>>>>
>>>>> In the process FreeBSD also gains plugin support (but untested).
>>>>>
>>>>> v4: - introduce error-early behavior for invalid plugin paths
>>>>>      - support directories via the existing -d option instead of adding new
>>>>>
>>>>> v3: - merge the first commits
>>>>>
>>>>> v2: - move code to eal/common
>>>>>      - add bsd support
>>>>>
>>>>> Panu Matilainen (2):
>>>>>    eal: move plugin loading to eal/common
>>>>>    eal: add support for driver directory concept
>>>>
>>>>
>>>> checkpatch complains for some indent problem (Thomas, can you fix this ?),
>>>> but the rest looks good to me.
>>>>
>>>> Acked-by: David Marchand <david.marchand@6wind.com>
>>>>
>>>> Thanks Panu.
>>>
>>> Applied, thanks
>>
>> This patch introduces a new issue reported by Coverity.
>>
>> The root cause of the problem is that you are checking that it s a directory first with stat
>> then calling dlopen(). I malicious entity could get between the stat and the dlopen.
>
> I think it is a false positive.
> The aim of loading every files in the directory is out of a security scope IMHO.
>

Yes its a false positive. The security aspect relates to world-writable 
directories and even in there the problem is usually "test for existence 
before creation", this is neither (if somebody routinely loads their 
critical device drivers from /tmp on a system they have bigger problems 
than this)

If somebody changes a file to a directory or vice versa then the 
consecutive readdir() or dlopen() on that entry will just fail, end of 
story. And if somebody has the permission to change entries in that 
directory they dont have to bother with trying to time their changes 
between stat() and dlopen().

Sure it could just call dlopen() on everything and if it fails try 
readdir() on it. Matter of style, I dislike blindly stumbling and 
crashing when I can simply take a look to see whether its a door, a 
window or a wall :)

	- Panu -

[PATCH v4 1/2] eal: move plugin loading to eal/common

[Panu Matilainen]

There's no good reason to limit plugins to Linux, make it available
on FreeBSD too. Refactor the plugin code from Linux EAL to common
helper functions, also check for and fail on errors during initialization.

Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 lib/librte_eal/bsdapp/eal/eal.c            |  3 ++
 lib/librte_eal/common/eal_common_options.c | 55 ++++++++++++++++++++++++++++++
 lib/librte_eal/common/eal_options.h        |  1 +
 lib/librte_eal/linuxapp/eal/eal.c          | 40 ++--------------------
 4 files changed, 61 insertions(+), 38 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index b64bbfc..a34e61d 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -571,6 +571,9 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
+	if (eal_plugins_init() < 0)
+		rte_panic("Cannot init plugins\n");
+
 	eal_thread_init_master(rte_config.master_lcore);
 
 	ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 79db608..ae0187a 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -39,6 +39,7 @@
 #include <limits.h>
 #include <errno.h>
 #include <getopt.h>
+#include <dlfcn.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>
@@ -94,6 +95,20 @@ eal_long_options[] = {
 	{0,                     0, NULL, 0                        }
 };
 
+TAILQ_HEAD(shared_driver_list, shared_driver);
+
+/* Definition for shared object drivers. */
+struct shared_driver {
+	TAILQ_ENTRY(shared_driver) next;
+
+	char    name[PATH_MAX];
+	void*   lib_handle;
+};
+
+/* List of external loadable drivers */
+static struct shared_driver_list solib_list =
+TAILQ_HEAD_INITIALIZER(solib_list);
+
 static int master_lcore_parsed;
 static int mem_parsed;
 
@@ -134,6 +149,40 @@ eal_reset_internal_config(struct internal_config *internal_cfg)
 	internal_cfg->create_uio_dev = 0;
 }
 
+static int
+eal_plugin_add(const char *path)
+{
+	struct shared_driver *solib;
+
+	solib = malloc(sizeof(*solib));
+	if (solib == NULL) {
+		RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
+		return -1;
+	}
+	memset(solib, 0, sizeof(*solib));
+	strncpy(solib->name, path, PATH_MAX-1);
+	solib->name[PATH_MAX-1] = 0;
+	TAILQ_INSERT_TAIL(&solib_list, solib, next);
+
+	return 0;
+}
+
+int
+eal_plugins_init(void)
+{
+	struct shared_driver *solib = NULL;
+
+	TAILQ_FOREACH(solib, &solib_list, next) {
+		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
+		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
+		if (solib->lib_handle == NULL) {
+			RTE_LOG(ERR, EAL, "%s\n", dlerror());
+			return -1;
+		}
+	}
+	return 0;
+}
+
 /*
  * Parse the coremask given as argument (hexadecimal string) and fill
  * the global configuration (core role and core count) with the parsed
@@ -713,6 +762,11 @@ eal_parse_common_option(int opt, const char *optarg,
 		 * even if info or warning messages are disabled */
 		RTE_LOG(CRIT, EAL, "RTE Version: '%s'\n", rte_version());
 		break;
+	/* force loading of external driver */
+	case 'd':
+		if (eal_plugin_add(optarg) == -1)
+			return -1;
+		break;
 
 	/* long options */
 	case OPT_HUGE_UNLINK_NUM:
@@ -898,6 +952,7 @@ eal_common_usage(void)
 	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
+	       "  -d LIB.so           Add driver (can be used multiple times)\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h
index 4245fd5..a881c62 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -95,5 +95,6 @@ int eal_adjust_config(struct internal_config *internal_cfg);
 int eal_check_common_options(struct internal_config *internal_cfg);
 void eal_common_usage(void);
 enum rte_proc_type_t eal_proc_type_detect(void);
+int eal_plugins_init(void);
 
 #endif /* EAL_OPTIONS_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 18fe19b..06536f2 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -43,7 +43,6 @@
 #include <getopt.h>
 #include <sys/file.h>
 #include <fcntl.h>
-#include <dlfcn.h>
 #include <stddef.h>
 #include <errno.h>
 #include <limits.h>
@@ -90,20 +89,6 @@
 /* Allow the application to print its usage message too if set */
 static rte_usage_hook_t	rte_application_usage_hook = NULL;
 
-TAILQ_HEAD(shared_driver_list, shared_driver);
-
-/* Definition for shared object drivers. */
-struct shared_driver {
-	TAILQ_ENTRY(shared_driver) next;
-
-	char    name[PATH_MAX];
-	void*   lib_handle;
-};
-
-/* List of external loadable drivers */
-static struct shared_driver_list solib_list =
-TAILQ_HEAD_INITIALIZER(solib_list);
-
 /* early configuration structure, when memory config is not mmapped */
 static struct rte_mem_config early_mem_config;
 
@@ -350,7 +335,6 @@ eal_usage(const char *prgname)
 	printf("\nUsage: %s ", prgname);
 	eal_common_usage();
 	printf("EAL Linux options:\n"
-	       "  -d LIB.so           Add driver (can be used multiple times)\n"
 	       "  --"OPT_SOCKET_MEM"        Memory to allocate on sockets (comma separated values)\n"
 	       "  --"OPT_HUGE_DIR"          Directory where hugetlbfs is mounted\n"
 	       "  --"OPT_FILE_PREFIX"       Prefix for hugepage filenames\n"
@@ -545,7 +529,6 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
-	struct shared_driver *solib;
 	const int old_optind = optind;
 	const int old_optopt = optopt;
 	char * const old_optarg = optarg;
@@ -579,20 +562,6 @@ eal_parse_args(int argc, char **argv)
 			eal_usage(prgname);
 			exit(EXIT_SUCCESS);
 
-		/* force loading of external driver */
-		case 'd':
-			solib = malloc(sizeof(*solib));
-			if (solib == NULL) {
-				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
-				ret = -1;
-				goto out;
-			}
-			memset(solib, 0, sizeof(*solib));
-			strncpy(solib->name, optarg, PATH_MAX-1);
-			solib->name[PATH_MAX-1] = 0;
-			TAILQ_INSERT_TAIL(&solib_list, solib, next);
-			break;
-
 		/* long options */
 		case OPT_XEN_DOM0_NUM:
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -758,7 +727,6 @@ rte_eal_init(int argc, char **argv)
 	int i, fctret, ret;
 	pthread_t thread_id;
 	static rte_atomic32_t run_once = RTE_ATOMIC32_INIT(0);
-	struct shared_driver *solib = NULL;
 	const char *logid;
 	char cpuset[RTE_CPU_AFFINITY_STR_LEN];
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
@@ -852,12 +820,8 @@ rte_eal_init(int argc, char **argv)
 
 	rte_eal_mcfg_complete();
 
-	TAILQ_FOREACH(solib, &solib_list, next) {
-		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
-		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-		if (solib->lib_handle == NULL)
-			RTE_LOG(WARNING, EAL, "%s\n", dlerror());
-	}
+	if (eal_plugins_init() < 0)
+		rte_panic("Cannot init plugins\n");
 
 	eal_thread_init_master(rte_config.master_lcore);
 
-- 
2.5.0

[PATCH v4 2/2] eal: add support for driver directory concept

[Panu Matilainen]

Add support for directories as arguments to -d for loading all drivers
from a given directory. Additionally a default driver directory can be
set in build-time configuration, in which case it will be always be used
when EAL is initialized.

This simplifies usage in shared library configuration significantly over
manually loading individual drivers with -d, and allows distros to
establish a drop-in driver directory for seamless integration
with 3rd party drivers etc.

Suggested-by: Thomas Monjalon <thomas.monjalon@6wind.com>
Suggested-by: David Marchand <david.marchand@6wind.com>
Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
---
 config/common_bsdapp                       |  3 ++
 config/common_linuxapp                     |  3 ++
 lib/librte_eal/common/eal_common_options.c | 74 ++++++++++++++++++++++++++++--
 3 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index fba29e5..7df0763 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -108,6 +108,9 @@ CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
 CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # FreeBSD contiguous memory driver settings
 #
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 7248262..52173d5 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -111,6 +111,9 @@ CONFIG_RTE_EAL_IGB_UIO=y
 CONFIG_RTE_EAL_VFIO=y
 CONFIG_RTE_MALLOC_DEBUG=n
 
+# Default driver path (or "" to disable)
+CONFIG_RTE_EAL_PMD_PATH=""
+
 #
 # Special configurations in PCI Config Space for high performance
 #
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index ae0187a..a2c3d91 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -40,6 +40,9 @@
 #include <errno.h>
 #include <getopt.h>
 #include <dlfcn.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <dirent.h>
 
 #include <rte_eal.h>
 #include <rte_log.h>
@@ -109,6 +112,9 @@ struct shared_driver {
 static struct shared_driver_list solib_list =
 TAILQ_HEAD_INITIALIZER(solib_list);
 
+/* Default path of external loadable drivers */
+static const char *default_solib_dir = RTE_EAL_PMD_PATH;
+
 static int master_lcore_parsed;
 static int mem_parsed;
 
@@ -167,18 +173,75 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
+static int
+eal_plugindir_init(const char *path)
+{
+	DIR *d = NULL;
+	struct dirent *dent = NULL;
+	char sopath[PATH_MAX];
+
+	if (path == NULL || *path == '\0')
+		return 0;
+
+	d = opendir(path);
+	if (d == NULL) {
+		RTE_LOG(ERR, EAL, "failed to open directory %s: %s\n",
+			path, strerror(errno));
+		return -1;
+	}
+
+	while ((dent = readdir(d)) != NULL) {
+		if (dent->d_type != DT_REG && dent->d_type != DT_LNK)
+		continue;
+
+		snprintf(sopath, PATH_MAX-1, "%s/%s", path, dent->d_name);
+		sopath[PATH_MAX-1] = 0;
+
+		if (eal_plugin_add(sopath) == -1)
+		    break;
+	}
+
+	closedir(d);
+	/* XXX this ignores failures from readdir() itself */
+	return (dent == NULL) ? 0 : -1;
+}
+
 int
 eal_plugins_init(void)
 {
 	struct shared_driver *solib = NULL;
 
+	if (*default_solib_dir != '\0')
+		eal_plugin_add(default_solib_dir);
+
 	TAILQ_FOREACH(solib, &solib_list, next) {
-		RTE_LOG(DEBUG, EAL, "open shared lib %s\n", solib->name);
-		solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-		if (solib->lib_handle == NULL) {
-			RTE_LOG(ERR, EAL, "%s\n", dlerror());
+		struct stat sb;
+		if (stat(solib->name, &sb) == -1) {
+			RTE_LOG(ERR, EAL, "Invalid plugin specified: %s: %s\n",
+				solib->name, strerror(errno));
 			return -1;
 		}
+
+		switch (sb.st_mode & S_IFMT) {
+		case S_IFDIR:
+			if (eal_plugindir_init(solib->name) == -1) {
+				RTE_LOG(ERR, EAL,
+					"Cannot init plugin directory %s\n",
+					solib->name);
+				return -1;
+			}
+			break;
+		case S_IFREG:
+			RTE_LOG(DEBUG, EAL, "open shared lib %s\n",
+				solib->name);
+			solib->lib_handle = dlopen(solib->name, RTLD_NOW);
+			if (solib->lib_handle == NULL) {
+				RTE_LOG(ERR, EAL, "%s\n", dlerror());
+				return -1;
+			}
+			break;
+		}
+
 	}
 	return 0;
 }
@@ -952,7 +1015,8 @@ eal_common_usage(void)
 	       "  --"OPT_PROC_TYPE"         Type of this process (primary|secondary|auto)\n"
 	       "  --"OPT_SYSLOG"            Set syslog facility\n"
 	       "  --"OPT_LOG_LEVEL"         Set default log level\n"
-	       "  -d LIB.so           Add driver (can be used multiple times)\n"
+	       "  -d LIB.so|DIR       Add a driver or driver directory\n"
+	       "                      (can be used multiple times)\n"
 	       "  -v                  Display version information on startup\n"
 	       "  -h, --help          This help\n"
 	       "\nEAL options for DEBUG use only:\n"
-- 
2.5.0