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

[David Marchand <david.marchand@6wind.com>]

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