Add support for driver directories

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(-)

Hello Panu,

On Fri, Sep 25, 2015 at 1:58 PM, Panu Matilainen <[email protected] <mailto:[email protected]>> 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 ?

Quoted 21 lines · David Marchand

Hello Panu,

On Fri, Sep 25, 2015 at 1:58 PM, Panu Matilainen <[email protected] <mailto:[email protected]>> 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(-)

Quoted 2 lines

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 :)

Quoted 8 lines · Panu Matilainen

Quoted 2 lines · David Marchand

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 :)

Signed-off-by Panu Matilainen <[email protected]>

---
 lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

lib/librte_eal/linuxapp/eal/eal.c +3 -0

index 33e1067..cc66d9f 100644
@@ -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)
+{

Signed-off-by Panu Matilainen <[email protected]>

---
 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(-)

Quoted 13 lines · Panu Matilainen

Signed-off-by Panu Matilainen <[email protected]>

---
  lib/librte_eal/linuxapp/eal/eal.c | 28 +++++++++++++++++++---------
  1 file changed, 19 insertions(+), 9 deletions(-)

lib/librte_eal/linuxapp/eal/eal.c +0 -0

index 33e1067..cc66d9f 100644
@@ -530,6 +530,24 @@ eal_log_level_parse(int argc, char **argv)
  	optind = 0; /* reset getopt lib */
  }
 

lib/librte_eal/linuxapp/eal/eal.c eal_log_level_parse(int argc, char **argv) · line 545

Quoted 13 lines

lib/librte_eal/linuxapp/eal/eal.c +13 -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;

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

Quoted 7 lines · Panu Matilainen

Signed-off-by Panu Matilainen <[email protected]>

---
  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 ?

Quoted 9 lines · Bruce Richardson

Quoted 7 lines · Panu Matilainen

Signed-off-by Panu Matilainen <[email protected]>

---
  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(-)

Quoted 3 lines

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

Quoted 14 lines · Bruce Richardson

Quoted 9 lines · Panu Matilainen

Quoted 7 lines

Signed-off-by Panu Matilainen <[email protected]>

---
  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(-)

Quoted 3 lines

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

Quoted 8 lines

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

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).

2015-10-21 11:29, Panu Matilainen:

Quoted 4 lines

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).

[...]

lib/librte_eal/common/eal_common_options.c eal_common_usage(void) · line 1007

Quoted 5 lines

lib/librte_eal/common/eal_common_options.c +1 -0

@@ -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?

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 <[email protected]>

---
 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(-)

lib/librte_eal/bsdapp/eal/eal.c +0 -0

index 1b6f705..f07a3c3 100644

Quoted 4 lines · 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.

Not sure about this "potential errors".

Quoted 17 lines

Signed-off-by Panu Matilainen <[email protected]>

---
  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(-)

lib/librte_eal/bsdapp/eal/eal.c +0 -0

index 1b6f705..f07a3c3 100644
@@ -543,6 +543,9 @@ rte_eal_init(int argc, char **argv)
 

lib/librte_eal/bsdapp/eal/eal.c rte_eal_init(int argc, char **argv) · line 550

Quoted 6 lines

lib/librte_eal/bsdapp/eal/eal.c +3 -0

         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) ?

lib/librte_eal/common/eal_common_options.c

Quoted 9 lines

lib/librte_eal/bsdapp/eal/eal.c +0 -0

         eal_thread_init_master(rte_config.master_lcore);
 

         ret = eal_thread_dump_affinity(cpuset, RTE_CPU_AFFINITY_STR_LEN);

lib/librte_eal/common/eal_common_options.c +0 -0

index 1f459ac..b542868 100644

[snip]

+

lib/librte_eal/common/eal_common_options.c eal_reset_internal_config(struct internal_config *internal_cfg) · line 182

Quoted 13 lines

lib/librte_eal/common/eal_common_options.c +13 -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;
+}

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.

Btw, returning an error here would change current behavior of dpdk loading

Quoted 3 lines

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.

Quoted 12 lines · Panu Matilainen

Btw, returning an error here would change current behavior of dpdk loading

Quoted 3 lines

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.

Quoted 6 lines

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 ?

Quoted 21 lines

Quoted 18 lines · Panu Matilainen

Btw, returning an error here would change current behavior of dpdk loading

Quoted 3 lines

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.

Quoted 5 lines

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.

2015-10-21 12:15, Bruce Richardson:

Quoted 26 lines · David Marchand

Quoted 25 lines

Quoted 12 lines · Panu Matilainen

Btw, returning an error here would change current behavior of dpdk loading

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

Quoted 1 line

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.

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

Quoted 9 lines

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

Quoted 8 lines

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 <[email protected]>

Thanks Panu.

Quoted 19 lines

Quoted 17 lines

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

Quoted 6 lines

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

Acked-by David Marchand <[email protected]>

Thanks Panu.

Applied, thanks

Quoted 24 lines · On Thu, 12 Nov 2015 16:52:32 +0100 Thomas Monjalon <[email protected]>…

Quoted 24 lines

Quoted 9 lines

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

Quoted 8 lines

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 <[email protected]>

Quoted 3 lines

Quoted 1 line

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.

2015-12-02 18:07, Stephen Hemminger:

Quoted 31 lines

Quoted 27 lines · On Thu, 12 Nov 2015 16:52:32 +0100 Thomas Monjalon <[email protected]>…

Quoted 19 lines

Quoted 9 lines

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

Quoted 8 lines

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

Quoted 6 lines

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

Acked-by David Marchand <[email protected]>

Thanks Panu.

Applied, thanks

Quoted 4 lines

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.