From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 70D6C16E for ; Fri, 30 Jun 2017 18:04:52 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga105.fm.intel.com with ESMTP; 30 Jun 2017 09:04:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,287,1496127600"; d="scan'208";a="1146538868" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.28]) by orsmga001.jf.intel.com with SMTP; 30 Jun 2017 09:04:48 -0700 Received: by (sSMTP sendmail emulation); Fri, 30 Jun 2017 17:04:46 +0100 Date: Fri, 30 Jun 2017 17:04:45 +0100 From: Bruce Richardson To: Jacek Piasecki Cc: dev@dpdk.org, deepak.k.jain@intel.com, Kuba Kozak Message-ID: <20170630160445.GA17400@bricha3-MOBL3.ger.corp.intel.com> References: <1498474759-102089-6-git-send-email-jacekx.piasecki@intel.com> <1498560760-104196-1-git-send-email-jacekx.piasecki@intel.com> <1498560760-104196-2-git-send-email-jacekx.piasecki@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1498560760-104196-2-git-send-email-jacekx.piasecki@intel.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.1 (2017-04-11) Subject: Re: [dpdk-dev] [PATCH v3 1/3] eal: add functions parsing EAL arguments X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Jun 2017 16:04:53 -0000 On Tue, Jun 27, 2017 at 12:52:38PM +0200, Jacek Piasecki wrote: > From: Kuba Kozak > > added function rte_eal_configure which configure > Environment Abstraction Layer (EAL) using > configuration structure. > > Signed-off-by: Kuba Kozak > Suggested-by: Bruce Richardson > --- 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 > #include > #include > +#include > > #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 > #include > > +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 > #include > #include > +#ifdef RTE_LIBRTE_CFGFILE > +#include > +#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 >