On Tue, Jun 27, 2017 at 12:52:38PM +0200, Jacek Piasecki wrote:
> From: Kuba Kozak <kubax.ko...@intel.com>
> 
> added function rte_eal_configure which configure
> Environment Abstraction Layer (EAL) using
> configuration structure.
> 
> Signed-off-by: Kuba Kozak <kubax.ko...@intel.com>
> Suggested-by: Bruce Richardson <bruce.richard...@intel.com>
> ---

Thanks for looking to implement this idea, to see how it would work.
Comments on the implementation inline below.

Regards,
/Bruce

> This patch depends on cfgfile patchset with:
> "cfgfile: remove EAL dependency"
> "cfgfile: add new functions to API"
> "cfgfile: rework of load function"
> "test/cfgfile: add new unit test"
> ---
>  config/common_base                              |   1 +
>  lib/Makefile                                    |   6 +-
>  lib/librte_eal/bsdapp/eal/Makefile              |   4 +
>  lib/librte_eal/bsdapp/eal/eal.c                 | 249 ++++++++++++++---
>  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |   4 +
>  lib/librte_eal/common/eal_common_cpuflags.c     |  14 +-
>  lib/librte_eal/common/eal_common_lcore.c        |  11 +-
>  lib/librte_eal/common/eal_common_options.c      |   5 +
>  lib/librte_eal/common/include/rte_eal.h         |  21 ++
>  lib/librte_eal/linuxapp/eal/Makefile            |   3 +
>  lib/librte_eal/linuxapp/eal/eal.c               | 353 
> ++++++++++++++++++------
>  lib/librte_eal/linuxapp/eal/rte_eal_version.map |   4 +
>  mk/rte.app.mk                                   |   2 +-
>  13 files changed, 543 insertions(+), 134 deletions(-)
> 
> diff --git a/config/common_base b/config/common_base
> index f6aafd1..c1d0e69 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -569,6 +569,7 @@ CONFIG_RTE_LIBRTE_TIMER_DEBUG=n
>  # Compile librte_cfgfile
>  #
>  CONFIG_RTE_LIBRTE_CFGFILE=y
> +CONFIG_RTE_LIBRTE_CFGFILE_DEBUG=n

This change does not belong in this patchset. In fact, I don't think it
belongs anywhere as there are no debug statements in the cfgfile
library.

>  
>  #
>  # Compile librte_cmdline
> diff --git a/lib/Makefile b/lib/Makefile
> index 07e1fd0..fc5df3a 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -32,7 +32,11 @@
>  include $(RTE_SDK)/mk/rte.vars.mk
>  
>  DIRS-y += librte_compat
> +DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
>  DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
> +ifeq ($(CONFIG_RTE_LIBRTE_CFGFILE),y)
> +DEPDIRS-librte_eal := librte_cfgfile
> +endif
>  DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
>  DEPDIRS-librte_ring := librte_eal
>  DIRS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += librte_mempool
> @@ -41,8 +45,6 @@ DIRS-$(CONFIG_RTE_LIBRTE_MBUF) += librte_mbuf
>  DEPDIRS-librte_mbuf := librte_eal librte_mempool
>  DIRS-$(CONFIG_RTE_LIBRTE_TIMER) += librte_timer
>  DEPDIRS-librte_timer := librte_eal
> -DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
> -DEPDIRS-librte_cfgfile := librte_eal
>  DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
>  DEPDIRS-librte_cmdline := librte_eal
>  DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether

I think the parts of this change - the deleteion of the line saying
cfgfile depends on EAL, and the moving of the declaration of the cfgfile
library up in the file, should go in patch 1 of the earlier cfgfile set.

> diff --git a/lib/librte_eal/bsdapp/eal/Makefile 
> b/lib/librte_eal/bsdapp/eal/Makefile
> index a0f9950..d70eefb 100644
> --- a/lib/librte_eal/bsdapp/eal/Makefile
> +++ b/lib/librte_eal/bsdapp/eal/Makefile
> @@ -50,6 +50,10 @@ EXPORT_MAP := rte_eal_version.map
>  
>  LIBABIVER := 4
>  
> +ifeq ($(CONFIG_RTE_LIBRTE_CFGFILE),y)
> +LDLIBS += -lrte_cfgfile
> +endif
> +

This should not be needed here. Other DPDK libs which depend on yet
other libs don't go modifying the LDLIBS like this.

>  # specific to bsdapp exec-env
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) := eal.c
>  SRCS-$(CONFIG_RTE_EXEC_ENV_BSDAPP) += eal_memory.c
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 05f0c1f..7baf848 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -73,6 +73,7 @@
>  #include <rte_version.h>
>  #include <rte_atomic.h>
>  #include <malloc_heap.h>
> +#include <rte_cfgfile.h>
>  
>  #include "eal_private.h"
>  #include "eal_thread.h"
> @@ -309,6 +310,18 @@ eal_get_hugepage_mem_size(void)
>  
>  /* Parse the arguments for --log-level only */
>  static void
> +eal_log_level_cfg(struct rte_cfgfile *cfg)
> +{
> +     const char *entry;
> +
> +     entry = rte_cfgfile_get_entry(cfg, "DPDK", OPT_LOG_LEVEL);
> +     if (entry)
> +             eal_parse_common_option(OPT_LOG_LEVEL_NUM, entry,
> +                             &internal_config);
> +}
> +
> +/* Parse the arguments for --log-level only */
> +static void
>  eal_log_level_parse(int argc, char **argv)
>  {
>       int opt;
> @@ -347,6 +360,58 @@ eal_log_level_parse(int argc, char **argv)
>       optarg = old_optarg;
>  }
>  
> +/* Parse single argument */
> +static int
> +eal_parse_option(int opt, char *optarg, int option_index, char *prgname)
> +{
> +     int ret;
> +
> +     /* getopt is not happy, stop right now */
> +     if (opt == '?') {
> +             eal_usage(prgname);
> +             ret = -1;
> +             goto out;
> +     }
> +
> +     ret = eal_parse_common_option(opt, optarg, &internal_config);
> +     /* common parser is not happy */
> +     if (ret < 0) {
> +             eal_usage(prgname);
> +             ret = -1;
> +             goto out;
> +     }
> +     /* common parser handled this option */
> +     if (ret == 0)
> +             return 0;
> +
> +     switch (opt) {
> +     case 'h':
> +             eal_usage(prgname);
> +             exit(EXIT_SUCCESS);
> +             break;
> +
> +     default:
> +             if (opt < OPT_LONG_MIN_NUM && isprint(opt)) {
> +                     RTE_LOG(ERR, EAL, "Option %c is not supported "
> +                             "on FreeBSD\n", opt);
> +             } else if (opt >= OPT_LONG_MIN_NUM &&
> +                        opt < OPT_LONG_MAX_NUM) {
> +                     RTE_LOG(ERR, EAL, "Option %s is not supported "
> +                             "on FreeBSD\n",
> +                             eal_long_options[option_index].name);
> +             } else {
> +                     RTE_LOG(ERR, EAL, "Option %d is not supported "
> +                             "on FreeBSD\n", opt);
> +             }
> +             eal_usage(prgname);
> +             ret = -1;
> +             goto out;
> +     }
> +     return 0;
> +out:
> +     return ret;
> +}
> +
>  /* Parse the argument given in the command line of the application */
>  static int
>  eal_parse_args(int argc, char **argv)
> @@ -367,45 +432,9 @@ eal_parse_args(int argc, char **argv)
>       while ((opt = getopt_long(argc, argvopt, eal_short_options,
>                                 eal_long_options, &option_index)) != EOF) {
>  
> -             /* getopt is not happy, stop right now */
> -             if (opt == '?') {
> -                     eal_usage(prgname);
> -                     ret = -1;
> -                     goto out;
> -             }
> -
> -             ret = eal_parse_common_option(opt, optarg, &internal_config);
> -             /* common parser is not happy */
> -             if (ret < 0) {
> -                     eal_usage(prgname);
> -                     ret = -1;
> -                     goto out;
> -             }
> -             /* common parser handled this option */
> -             if (ret == 0)
> -                     continue;
> -
> -             switch (opt) {
> -             case 'h':
> -                     eal_usage(prgname);
> -                     exit(EXIT_SUCCESS);
> -             default:
> -                     if (opt < OPT_LONG_MIN_NUM && isprint(opt)) {
> -                             RTE_LOG(ERR, EAL, "Option %c is not supported "
> -                                     "on FreeBSD\n", opt);
> -                     } else if (opt >= OPT_LONG_MIN_NUM &&
> -                                opt < OPT_LONG_MAX_NUM) {
> -                             RTE_LOG(ERR, EAL, "Option %s is not supported "
> -                                     "on FreeBSD\n",
> -                                     eal_long_options[option_index].name);
> -                     } else {
> -                             RTE_LOG(ERR, EAL, "Option %d is not supported "
> -                                     "on FreeBSD\n", opt);
> -                     }
> -                     eal_usage(prgname);
> -                     ret = -1;
> +             ret = eal_parse_option(opt, optarg, option_index, prgname);
> +             if (ret < 0)
>                       goto out;
> -             }
>       }
>  
>       if (eal_adjust_config(&internal_config) != 0) {
> @@ -677,3 +706,147 @@ rte_eal_process_type(void)
>  {
>       return rte_config.process_type;
>  }
> +
> +#ifdef RTE_LIBRTE_CFGFILE
> +#define vdev_buff_size               200
> +#define sectionname_size     20
> +static int
> +parse_vdev_devices(struct rte_cfgfile *cfg)
> +{
> +     char sectionname[sectionname_size];
> +     char buffer1[vdev_buff_size];
> +     int vdev_nb = 0;
> +     int n_entries;
> +     int i;
> +
> +     /* ----------- parsing VDEVS */
> +     snprintf(sectionname, sectionname_size, "DPDK.vdev%d", vdev_nb);

move this inside the loop to be the first thing done, rather than having
it outside and then the last thing on each iteration.

> +
> +     for (vdev_nb = 1; rte_cfgfile_has_section(cfg, sectionname);
> +                     vdev_nb++) {
> +             n_entries = rte_cfgfile_section_num_entries(cfg, sectionname);
> +
> +             struct rte_cfgfile_entry entries[n_entries];
> +
> +             if (n_entries != rte_cfgfile_section_entries(cfg, sectionname,
> +                             entries, n_entries)) {
> +                     rte_eal_init_alert("Unexpected fault.");
> +                     rte_errno = EFAULT;
> +                     return -1;
> +             }
> +
> +             buffer1[0] = 0;
> +             for (i = 0; i < n_entries; i++) {
> +                     if (strlen(entries[i].value)) {
> +
> +                             if ((strlen(buffer1) +
> +                                             strlen(entries[i].name) +
> +                                             strlen(entries[i].value) + 3)
> +                                             >= vdev_buff_size)
> +                                     goto buff_size_err;
> +                             strcat(buffer1, entries[i].name);
> +                             strcat(buffer1, "=");
> +                             strcat(buffer1, entries[i].value);
> +                     } else {
> +                             if ((strlen(buffer1) +
> +                                             strlen(entries[i].name) + 2)
> +                                             >= vdev_buff_size)
> +                                     goto buff_size_err;
> +                             strcat(buffer1, entries[i].name);
> +                     }
> +

Doing these strlen calls and calculations that way is horribly
inefficient IMHO, even if this is not in a perf-critical path. Instead,
I would look to start with a buf_len variable which tracks the available
buffer size, and a buf_ptr variable which points to the first available
character. Then do the following each time you want to add something:

        ret = snprintf(buf_ptr, buf_len, "%s%s%s", entries[i].name,
                entries[i].value[0] != '\0' ? "=" : "",
                entries[i].value);
        if (ret >= buf_len)
                goto buff_size_err;
        buf_ptr += ret;
        buf_len -= ret;

Shorter and simpler, as well as being more efficient as it does not need
any string length computation calls, using snprintf safely instead.

> +                     if (i < (n_entries - 1))
> +                             strcat(buffer1, ",");

You can add this bit into the above snprintf too, in a similar way to
how I optionally added the "=".

> +             }
> +
> +             /* parsing vdev */
> +             if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
> +                             buffer1) < 0) {
> +                     return -1;
> +             }
> +             snprintf(sectionname, sectionname_size, "DPDK.vdev%d", vdev_nb);
> +     }
> +     /* ----------- parsing VDEVS */
> +     return 0;
> +
> +buff_size_err:
> +     printf("parse_vdev_devices(): buffer size is to small\n");
> +     return -1;

If the code is reworked so that there is only one place where this error
occurs, remove the label and goto, and just put the error handling
inline in the code.

> +}
> +
> +static void
> +eal_getopt(const char *str, int *opt, int *option_index)
> +{
> +     int i;
> +
> +     *opt = '?';
> +     *option_index = 0;
> +
> +     if (strlen(str) == 1) {
> +             *opt = *str;
> +             return;
> +     }
> +
> +     for (i = 0; eal_long_options[i].name != NULL; i++) {
> +             if (strcmp(str, eal_long_options[i].name) == 0) {
> +                     *opt = eal_long_options[i].val;
> +                     *option_index = i;
> +                     break;
> +             }
> +     }
> +}
> +
> +int
> +rte_eal_configure(struct rte_cfgfile *cfg, char *prgname)
> +{
> +     int n_entries;
> +     int i;
> +     int opt;
> +     int option_index;
> +
> +     if (cfg == NULL) {
> +             rte_errno = -EINVAL;
> +             return -1;
> +     }
> +
> +     n_entries = rte_cfgfile_section_num_entries(cfg, "DPDK");
> +
> +     if (n_entries < 1) {
> +             printf("No DPDK section entries in cfgfile object\n");
> +             return 0;
> +     }
> +
> +     struct rte_cfgfile_entry entries[n_entries];
> +
> +     if (n_entries !=
> +                     rte_cfgfile_section_entries(cfg, "DPDK", entries,
> +                                     n_entries)) {
> +             rte_eal_init_alert("Unexpected fault.");
> +             rte_errno = EFAULT;
> +             return -1;
> +     }
> +
> +     eal_reset_internal_config(&internal_config);
> +
> +     /* set log level as early as possible */
> +     eal_log_level_cfg(cfg);
> +
> +     for (i = 0; i < n_entries; i++) {
> +             eal_getopt(entries[i].name, &opt, &option_index);
> +
> +             if (eal_parse_option(opt, entries[i].value,
> +                             option_index, prgname) != 0) {
> +                     rte_eal_init_alert("Invalid config file arguments.");
> +                     rte_errno = EINVAL;
> +                     return -1;
> +             }
> +     }
> +
> +     if (parse_vdev_devices(cfg) < 0) {
> +             rte_eal_init_alert("Couldn't parse vdevs");
> +             rte_errno = ENOMEM;
> +             return -1;
> +     }
> +     return 0;
> +}
> +#endif
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map 
> b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 2e48a73..a939b03 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -193,3 +193,7 @@ DPDK_17.05 {
>       vfio_get_group_no;
>  
>  } DPDK_17.02;
> +
> +DPDK_17.08 {
> +     rte_eal_configure;
> +} DPDK_17.05;
> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c 
> b/lib/librte_eal/common/eal_common_cpuflags.c
> index 9a2d080..6a365f3 100644
> --- a/lib/librte_eal/common/eal_common_cpuflags.c
> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> @@ -50,12 +50,18 @@ rte_cpu_check_supported(void)
>  int
>  rte_cpu_is_supported(void)
>  {
> +     static int run_once;
> +     static int ret;
>       /* This is generated at compile-time by the build system */
>       static const enum rte_cpu_flag_t compile_time_flags[] = {
>                       RTE_COMPILE_TIME_CPUFLAGS
>       };
>       unsigned count = RTE_DIM(compile_time_flags), i;
> -     int ret;
> +
> +     /* No need to calculate this function again if we know the result */
> +     if (run_once)
> +             return ret;
> +     run_once = 1;
>  
>       for (i = 0; i < count; i++) {
>               ret = rte_cpu_get_flag_enabled(compile_time_flags[i]);
> @@ -64,16 +70,16 @@ rte_cpu_is_supported(void)
>                       fprintf(stderr,
>                               "ERROR: CPU feature flag lookup failed with 
> error %d\n",
>                               ret);
> -                     return 0;
> +                     return ret = 0;
>               }
>               if (!ret) {
>                       fprintf(stderr,
>                               "ERROR: This system does not support \"%s\".\n"
>                               "Please check that RTE_MACHINE is set 
> correctly.\n",
>                               rte_cpu_get_flag_name(compile_time_flags[i]));
> -                     return 0;
> +                     return ret = 0;
>               }
>       }
>  
> -     return 1;
> +     return ret = 1;
>  }

This is not a bad change in and of itself. However, I'm not sure it
belongs in this patchset. With the new configure call, we should still
only be calling these initialization functions once - in eal_init().


> diff --git a/lib/librte_eal/common/eal_common_lcore.c 
> b/lib/librte_eal/common/eal_common_lcore.c
> index 84fa0cb..ce3ef34 100644
> --- a/lib/librte_eal/common/eal_common_lcore.c
> +++ b/lib/librte_eal/common/eal_common_lcore.c
> @@ -53,11 +53,18 @@
>  int
>  rte_eal_cpu_init(void)
>  {
> +     static int run_once;
> +     static int ret;
>       /* pointer to global configuration */
>       struct rte_config *config = rte_eal_get_configuration();
>       unsigned lcore_id;
>       unsigned count = 0;
>  
> +     /* No need to calculate this function again if we know the result */
> +     if (run_once)
> +             return ret;
> +     run_once = 1;
> +
>       /*
>        * Parse the maximum set of logical cores, detect the subset of running
>        * ones and enable them by default.
> @@ -91,7 +98,7 @@ rte_eal_cpu_init(void)
>                               "RTE_MAX_NUMA_NODES (%d)\n",
>                               lcore_config[lcore_id].socket_id,
>                               RTE_MAX_NUMA_NODES);
> -                     return -1;
> +                     return ret = -1;
>  #endif
>               }
>               RTE_LOG(DEBUG, EAL, "Detected lcore %u as "
> @@ -107,5 +114,5 @@ rte_eal_cpu_init(void)
>               RTE_MAX_LCORE);
>       RTE_LOG(INFO, EAL, "Detected %u lcore(s)\n", config->lcore_count);
>  
> -     return 0;
> +     return ret = 0;
>  }

As above, should not be needed.

> diff --git a/lib/librte_eal/common/eal_common_options.c 
> b/lib/librte_eal/common/eal_common_options.c
> index f470195..138a27f 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -131,8 +131,13 @@ static int core_parsed;
>  void
>  eal_reset_internal_config(struct internal_config *internal_cfg)
>  {
> +     static int run_once;
>       int i;
>  
> +     if (run_once)
> +             return;
> +     run_once = 1;
> +
>       internal_cfg->memory = 0;
>       internal_cfg->force_nrank = 0;
>       internal_cfg->force_nchannel = 0;

I don't think for this function this is the way to do things, given that
the function does one job - resetting the internal config - and the new
flag just disables it. It's different from e.g. the cpu check which is
able to cache a return value.

Instead, this should be tracked at the higher eal level, and not call
the function at all if it is unneeded. This mean that eal_init should
check itself if configure has already called the function and then skip
calling it itself.

> diff --git a/lib/librte_eal/common/include/rte_eal.h 
> b/lib/librte_eal/common/include/rte_eal.h
> index abf020b..e0705de 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -46,6 +46,8 @@
>  #include <rte_per_lcore.h>
>  #include <rte_config.h>
>  
> +struct rte_cfgfile; /* forward declaration of struct */
> +
>  #ifdef __cplusplus
>  extern "C" {
>  #endif
> @@ -188,6 +190,25 @@ int rte_eal_iopl_init(void);
>   */
>  int rte_eal_init(int argc, char **argv);
>  
> +#ifdef RTE_LIBRTE_CFGFILE
> +/**
> + * Initialize the Environment Abstraction Layer (EAL) using
> + * configuration structure
> + *
> + * @param cfg
> + *   pointer to config file structure.
> + *   If 0 - function free allocated for **cfg_argv memory

Don't understand this comment.

> + * @param prgname
> + *   pointer to string with execution path
> + *
> + * @return
> + *  - On success, return 0
> + *  - On failure, returns -1.
> + */
> +int
> +rte_eal_configure(struct rte_cfgfile *cfg, char *prgname);
> +#endif
> +
>  /**
>   * Check if a primary process is currently alive
>   *
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile 
> b/lib/librte_eal/linuxapp/eal/Makefile
> index 640afd0..656033e 100644
> --- a/lib/librte_eal/linuxapp/eal/Makefile
> +++ b/lib/librte_eal/linuxapp/eal/Makefile
> @@ -50,6 +50,9 @@ LDLIBS += -ldl
>  LDLIBS += -lpthread
>  LDLIBS += -lgcc_s
>  LDLIBS += -lrt
> +ifeq ($(CONFIG_RTE_LIBRTE_CFGFILE),y)
> +LDLIBS += -lrte_cfgfile
> +endif
>  
>  # specific to linuxapp exec-env
>  SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) := eal.c
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
> b/lib/librte_eal/linuxapp/eal/eal.c
> index 7c78f2d..f5973f4 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -78,6 +78,9 @@
>  #include <rte_version.h>
>  #include <rte_atomic.h>
>  #include <malloc_heap.h>
> +#ifdef RTE_LIBRTE_CFGFILE
> +#include <rte_cfgfile.h>
> +#endif
>  
>  #include "eal_private.h"
>  #include "eal_thread.h"
> @@ -478,6 +481,20 @@ eal_parse_vfio_intr(const char *mode)
>       return -1;
>  }
>  
> +#ifdef RTE_LIBRTE_CFGFILE
> +/* Parse the arguments for --log-level only */
> +static void
> +eal_log_level_cfg(struct rte_cfgfile *cfg)
> +{
> +     const char *entry;
> +
> +     entry = rte_cfgfile_get_entry(cfg, "DPDK", OPT_LOG_LEVEL);
> +     if (entry)
> +             eal_parse_common_option(OPT_LOG_LEVEL_NUM, entry,
> +                             &internal_config);
> +}
> +#endif
> +

This looks the same as the BSD version. Can it go in a common file?

>  /* Parse the arguments for --log-level only */
>  static void
>  eal_log_level_parse(int argc, char **argv)
> @@ -515,119 +532,135 @@ eal_log_level_parse(int argc, char **argv)
>       optarg = old_optarg;
>  }
>  
> -/* Parse the argument given in the command line of the application */
> +/* Parse single argument */
>  static int
> -eal_parse_args(int argc, char **argv)
> +eal_parse_option(int opt, char *optarg, int option_index, char *prgname)
>  {
> -     int opt, ret;
> -     char **argvopt;
> -     int option_index;
> -     char *prgname = argv[0];
> -     const int old_optind = optind;
> -     const int old_optopt = optopt;
> -     char * const old_optarg = optarg;
> +     int ret;
>  
> -     argvopt = argv;
> -     optind = 1;
> +     /* getopt is not happy, stop right now */
> +     if (opt == '?') {
> +             eal_usage(prgname);
> +             ret = -1;
> +             goto out;
> +     }
>  
> -     while ((opt = getopt_long(argc, argvopt, eal_short_options,
> -                               eal_long_options, &option_index)) != EOF) {
> +     ret = eal_parse_common_option(opt, optarg, &internal_config);
> +     /* common parser is not happy */
> +     if (ret < 0) {
> +             eal_usage(prgname);
> +             ret = -1;
> +             goto out;
> +     }
> +     /* common parser handled this option */
> +     if (ret == 0)
> +             return 0;
>  
> -             /* getopt is not happy, stop right now */
> -             if (opt == '?') {
> +     switch (opt) {
> +     case 'h':
> +             eal_usage(prgname);
> +             exit(EXIT_SUCCESS);
> +             break;
> +
> +     /* long options */
> +     case OPT_XEN_DOM0_NUM:
> +#ifdef RTE_LIBRTE_XEN_DOM0
> +             internal_config.xen_dom0_support = 1;
> +             break;
> +#else
> +             RTE_LOG(ERR, EAL, "Can't support DPDK app "
> +                     "running on Dom0, please configure"
> +                     " RTE_LIBRTE_XEN_DOM0=y\n");
> +             ret = -1;
> +             goto out;
> +#endif
> +
> +     case OPT_HUGE_DIR_NUM:
> +             internal_config.hugepage_dir = optarg;
> +             break;
> +
> +     case OPT_FILE_PREFIX_NUM:
> +             internal_config.hugefile_prefix = optarg;
> +             break;
> +
> +     case OPT_SOCKET_MEM_NUM:
> +             if (eal_parse_socket_mem(optarg) < 0) {
> +                     RTE_LOG(ERR, EAL, "invalid parameters for --"
> +                                     OPT_SOCKET_MEM "\n");
>                       eal_usage(prgname);
>                       ret = -1;
>                       goto out;
>               }
> +             break;
>  
> -             ret = eal_parse_common_option(opt, optarg, &internal_config);
> -             /* common parser is not happy */
> -             if (ret < 0) {
> +     case OPT_BASE_VIRTADDR_NUM:
> +             if (eal_parse_base_virtaddr(optarg) < 0) {
> +                     RTE_LOG(ERR, EAL, "invalid parameter for --"
> +                                     OPT_BASE_VIRTADDR "\n");
>                       eal_usage(prgname);
>                       ret = -1;
>                       goto out;
>               }
> -             /* common parser handled this option */
> -             if (ret == 0)
> -                     continue;
> +             break;
>  
> -             switch (opt) {
> -             case 'h':
> +     case OPT_VFIO_INTR_NUM:
> +             if (eal_parse_vfio_intr(optarg) < 0) {
> +                     RTE_LOG(ERR, EAL, "invalid parameters for --"
> +                                     OPT_VFIO_INTR "\n");
>                       eal_usage(prgname);
> -                     exit(EXIT_SUCCESS);
> -
> -             /* long options */
> -             case OPT_XEN_DOM0_NUM:
> -#ifdef RTE_LIBRTE_XEN_DOM0
> -                     internal_config.xen_dom0_support = 1;
> -#else
> -                     RTE_LOG(ERR, EAL, "Can't support DPDK app "
> -                             "running on Dom0, please configure"
> -                             " RTE_LIBRTE_XEN_DOM0=y\n");
>                       ret = -1;
>                       goto out;
> -#endif
> -                     break;
> +             }
> +             break;
>  
> -             case OPT_HUGE_DIR_NUM:
> -                     internal_config.hugepage_dir = optarg;
> -                     break;
> +     case OPT_CREATE_UIO_DEV_NUM:
> +             internal_config.create_uio_dev = 1;
> +             break;
>  
> -             case OPT_FILE_PREFIX_NUM:
> -                     internal_config.hugefile_prefix = optarg;
> -                     break;
> +     default:
> +             if (opt < OPT_LONG_MIN_NUM && isprint(opt)) {
> +                     RTE_LOG(ERR, EAL, "Option %c is not supported "
> +                             "on Linux\n", opt);
> +             } else if (opt >= OPT_LONG_MIN_NUM &&
> +                        opt < OPT_LONG_MAX_NUM) {
> +                     RTE_LOG(ERR, EAL, "Option %s is not supported "
> +                             "on Linux\n",
> +                             eal_long_options[option_index].name);
> +             } else {
> +                     RTE_LOG(ERR, EAL, "Option %d is not supported "
> +                             "on Linux\n", opt);
> +             }
> +             eal_usage(prgname);
> +             ret = -1;
> +             goto out;
> +     }
>  
> -             case OPT_SOCKET_MEM_NUM:
> -                     if (eal_parse_socket_mem(optarg) < 0) {
> -                             RTE_LOG(ERR, EAL, "invalid parameters for --"
> -                                             OPT_SOCKET_MEM "\n");
> -                             eal_usage(prgname);
> -                             ret = -1;
> -                             goto out;
> -                     }
> -                     break;
> +     return 0;
> +out:
> +     return ret;
> +}
>  
> -             case OPT_BASE_VIRTADDR_NUM:
> -                     if (eal_parse_base_virtaddr(optarg) < 0) {
> -                             RTE_LOG(ERR, EAL, "invalid parameter for --"
> -                                             OPT_BASE_VIRTADDR "\n");
> -                             eal_usage(prgname);
> -                             ret = -1;
> -                             goto out;
> -                     }
> -                     break;
> +/* Parse the argument given in the command line of the application */
> +static int
> +eal_parse_args(int argc, char **argv)
> +{
> +     int opt, ret;
> +     char **argvopt;
> +     int option_index;
> +     char *prgname = argv[0];
> +     const int old_optind = optind;
> +     const int old_optopt = optopt;
> +     char * const old_optarg = optarg;
>  
> -             case OPT_VFIO_INTR_NUM:
> -                     if (eal_parse_vfio_intr(optarg) < 0) {
> -                             RTE_LOG(ERR, EAL, "invalid parameters for --"
> -                                             OPT_VFIO_INTR "\n");
> -                             eal_usage(prgname);
> -                             ret = -1;
> -                             goto out;
> -                     }
> -                     break;
> +     argvopt = argv;
> +     optind = 1;
>  
> -             case OPT_CREATE_UIO_DEV_NUM:
> -                     internal_config.create_uio_dev = 1;
> -                     break;
> +     while ((opt = getopt_long(argc, argvopt, eal_short_options,
> +                               eal_long_options, &option_index)) != EOF) {
>  
> -             default:
> -                     if (opt < OPT_LONG_MIN_NUM && isprint(opt)) {
> -                             RTE_LOG(ERR, EAL, "Option %c is not supported "
> -                                     "on Linux\n", opt);
> -                     } else if (opt >= OPT_LONG_MIN_NUM &&
> -                                opt < OPT_LONG_MAX_NUM) {
> -                             RTE_LOG(ERR, EAL, "Option %s is not supported "
> -                                     "on Linux\n",
> -                                     eal_long_options[option_index].name);
> -                     } else {
> -                             RTE_LOG(ERR, EAL, "Option %d is not supported "
> -                                     "on Linux\n", opt);
> -                     }
> -                     eal_usage(prgname);
> -                     ret = -1;
> +             ret = eal_parse_option(opt, optarg, option_index, prgname);
> +             if (ret < 0)
>                       goto out;
> -             }
>       }
>  
>       if (eal_adjust_config(&internal_config) != 0) {
> @@ -995,3 +1028,149 @@ rte_eal_check_module(const char *module_name)
>       /* Module has been found */
>       return 1;
>  }
> +
> +#ifdef RTE_LIBRTE_CFGFILE
> +#define vdev_buff_size               200
> +#define sectionname_size     20
> +static int
> +parse_vdev_devices(struct rte_cfgfile *cfg)
> +{
> +     char sectionname[sectionname_size];
> +     char buffer1[vdev_buff_size];
> +     int vdev_nb = 0;
> +     int n_entries;
> +
> +     int i;
> +
> +     /* ----------- parsing VDEVS */
> +     snprintf(sectionname, sectionname_size, "DPDK.vdev%d", vdev_nb);
> +
> +     for (vdev_nb = 1; rte_cfgfile_has_section(cfg, sectionname);
> +                     vdev_nb++) {
> +             n_entries = rte_cfgfile_section_num_entries(cfg, sectionname);
> +
> +             struct rte_cfgfile_entry entries[n_entries];
> +
> +
> +             if (n_entries != rte_cfgfile_section_entries(cfg, sectionname,
> +                             entries, n_entries)) {
> +                     rte_eal_init_alert("Unexpected fault.");
> +                     rte_errno = EFAULT;
> +                     return -1;
> +             }
> +
> +             buffer1[0] = 0;
> +             for (i = 0; i < n_entries; i++) {
> +                     if (strlen(entries[i].value)) {
> +
> +                             if ((strlen(buffer1) +
> +                                             strlen(entries[i].name) +
> +                                             strlen(entries[i].value) + 3)
> +                                             >= vdev_buff_size)
> +                                     goto buff_size_err;
> +                             strcat(buffer1, entries[i].name);
> +                             strcat(buffer1, "=");
> +                             strcat(buffer1, entries[i].value);
> +                     } else {
> +                             if ((strlen(buffer1) +
> +                                             strlen(entries[i].name) + 2)
> +                                             >= vdev_buff_size)
> +                                     goto buff_size_err;
> +                             strcat(buffer1, entries[i].name);
> +                     }
> +
> +                     if (i < (n_entries - 1))
> +                             strcat(buffer1, ",");
> +             }
> +
> +             /* parsing vdev */
> +             if (rte_eal_devargs_add(RTE_DEVTYPE_VIRTUAL,
> +                             buffer1) < 0) {
> +                     return -1;
> +             }
> +             snprintf(sectionname, sectionname_size, "DPDK.vdev%d", vdev_nb);
> +     }
> +     /* ----------- parsing VDEVS */
> +     return 0;
> +
> +buff_size_err:
> +     printf("parse_vdev_devices(): buffer size is to small\n");
> +     return -1;
> +}
> +
> +static void
> +eal_getopt(const char *str, int *opt, int *option_index)
> +{
> +     int i;
> +
> +     *opt = '?';
> +     *option_index = 0;
> +
> +     if (strlen(str) == 1) {
> +             *opt = *str;
> +             return;
> +     }
> +
> +     for (i = 0; eal_long_options[i].name != NULL; i++) {
> +             if (strcmp(str, eal_long_options[i].name) == 0) {
> +                     *opt = eal_long_options[i].val;
> +                     *option_index = i;
> +                     break;
> +             }
> +     }
> +}
> +
> +int
> +rte_eal_configure(struct rte_cfgfile *cfg, char *prgname)
> +{
> +     int n_entries;
> +     int i;
> +     int opt;
> +     int option_index;
> +
> +     if (cfg == NULL) {
> +             rte_errno = -EINVAL;
> +             return -1;
> +     }
> +
> +     n_entries = rte_cfgfile_section_num_entries(cfg, "DPDK");
> +
> +     if (n_entries < 1) {
> +             printf("No DPDK section entries in cfgfile object\n");
> +             return 0;
> +     }
> +
> +     struct rte_cfgfile_entry entries[n_entries];
> +
> +     if (n_entries !=
> +                     rte_cfgfile_section_entries(cfg, "DPDK", entries,
> +                                     n_entries)) {
> +             rte_eal_init_alert("Unexpected fault.");
> +             rte_errno = EFAULT;
> +             return -1;
> +     }
> +
> +     eal_reset_internal_config(&internal_config);
> +
> +     /* set log level as early as possible */
> +     eal_log_level_cfg(cfg);
> +
> +     for (i = 0; i < n_entries; i++) {
> +             eal_getopt(entries[i].name, &opt, &option_index);
> +
> +             if (eal_parse_option(opt, entries[i].value,
> +                             option_index, prgname) != 0) {
> +                     rte_eal_init_alert("Invalid config file arguments.");
> +                     rte_errno = EINVAL;
> +                     return -1;
> +             }
> +     }
> +
> +     if (parse_vdev_devices(cfg) < 0) {
> +             rte_eal_init_alert("Couldn't parse vdevs");
> +             rte_errno = ENOMEM;
> +             return -1;
> +     }
> +     return 0;
> +}
> +#endif

Can this code in the #ifdef #endif block above go in a common file, as
again it looks very alike the BSD version? 

> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map 
> b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 670bab3..c93e6d9 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -198,3 +198,7 @@ DPDK_17.05 {
>       vfio_get_group_no;
>  
>  } DPDK_17.02;
> +
> +DPDK_17.08 {
> +     rte_eal_configure;
> +} DPDK_17.05;
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index bcaf1b3..642af92 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -80,7 +80,6 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
>  
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_EFD)            += -lrte_efd
> -_LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
>  
>  _LDLIBS-y += --whole-archive
>  
> @@ -96,6 +95,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_MEMPOOL)        += -lrte_mempool
>  _LDLIBS-$(CONFIG_RTE_DRIVER_MEMPOOL_RING)   += -lrte_mempool_ring
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_RING)           += -lrte_ring
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_EAL)            += -lrte_eal
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_CMDLINE)        += -lrte_cmdline
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_REORDER)        += -lrte_reorder
>  
> -- 
> 2.7.4
> 

Reply via email to