Dmitry Eremin-Solenikov(lumag) replied on github web page: platform/linux-generic/odp_libconfig.c line 37 @@ -0,0 +1,82 @@ +/* Copyright (c) 2018, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include "config.h" + +#include <stdlib.h> +#include <libconfig.h> + +#include <odp_internal.h> +#include <odp_debug_internal.h> +#include <odp_libconfig_internal.h> +#include <odp_libconfig_config.h> + +extern struct odp_global_data_s odp_global_data; + +int _odp_libconfig_init_global(void) +{ + char *filename; + config_t *config = &odp_global_data.libconfig_default_structure; + config_t *config_rt = &odp_global_data.libconfig_runtime_structure; + + config_init(config); + + if (!config_read_string(config, (const char *)odp_linux_conf)) { + ODP_ERR("Failed to read default config: %s(%d): %s\n", + config_error_file(config), config_error_line(config), + config_error_text(config)); + config_destroy(config); + return -1; + } + + odp_global_data.libconfig_default = config; + + filename = getenv("ODP_CONFIG_FILE");
Comment: It might be usefull to have "default" config file provided at ${sysconfdir}. > Dmitry Eremin-Solenikov(lumag) wrote: > Please use instead: > `Require.private: libconfig` >> Dmitry Eremin-Solenikov(lumag) wrote: >> Generated file should go to builddir, not to srcdir. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> Please use `AC_CONFIG_COMMANDS` to regenerate this header. Also there >>> should be a dependency line in respective Makefile.am to regenerate the >>> header. >>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>> Can we please have just two config structures here (default and running)? >>>> I wonder, if we can actually merge default with provided configurations >>>> into a single config structure? >>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>> No need to `AC_SUBST` them, as you are using PKG_CHECK_MODULES. >>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>> No, this is not necessary. One can override this setting using >>>>>> environment. >>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>> As the last resort you can load it and then call `config_file_write()` >>>>>>> from the helper app. I'd prefer instead if callers could provide >>>>>>> default values fro options that they require. >>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>> `config_t libconfig`can always be initialized, but in case >>>>>>>> `ODP_CONF_FILE` is not given, no configuration file is read. I'm not >>>>>>>> sure if all libconfig functions handle this properly without crashing >>>>>>>> and I'm playing it safe here. Maybe it's cleaner if I replace this >>>>>>>> with: >>>>>>>> ``` >>>>>>>> config_t *libconfig; >>>>>>>> config_t libconfig_structure; >>>>>>>> ``` >>>>>>>> >>>>>>>> Actually, this gave me an idea. It would probably make sense to save >>>>>>>> the `odp-linux.conf` contents to a C file during configure. This way >>>>>>>> we always have a valid default configuration and the default options >>>>>>>> have to be saved only to one location. Any suggestions what would be a >>>>>>>> good method to save the file during configure? >>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>> `ODP_LOG()` will just print if not overridded. The point is ODP >>>>>>>>> shouldn't be using `printf()` directly. >>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>> I would prefer changing OFP to match our naming convention. We use >>>>>>>>>> 'config' in several APIs, whereas 'conf' is used only internally. >>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>> This is just an internal helper to avoid having to reference to >>>>>>>>>>> odp_global_data when calling `config_lookup()`. It could actually >>>>>>>>>>> be removed altogether. >>>>>>>>>>> >>>>>>>>>>> Since we are using libconfig only internally, what's the benefit of >>>>>>>>>>> wrapping its functions (other than avoiding hard dependency)? We >>>>>>>>>>> are not wrapping e.g. openssl functions. Adding wrappers for the >>>>>>>>>>> aforementioned `_odp_config_lookup_int()` and >>>>>>>>>>> `_odp_config_lookup_string()` would be simple, but for example` >>>>>>>>>>> config_setting_lookup()`, which I'm using to find dpdk driver >>>>>>>>>>> specific options, uses libconfig data types which would also have >>>>>>>>>>> to be wrapped. >>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>> As we already require for example autoconf, libtool, and >>>>>>>>>>>> pkg-config, adding libconfig dependency shouldn't be a big issue. >>>>>>>>>>>> However, if this causes problems we can remove the hard dependency >>>>>>>>>>>> by wrapping all used libconfig functions with dummy >>>>>>>>>>>> implementations. I would prefer not doing this if not explicitly >>>>>>>>>>>> required. >>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>> Not that familiar with autoconf, so I pretty much copy-pasted the >>>>>>>>>>>>> `odp_openssl.m4` to get this working. I'll take a look at >>>>>>>>>>>>> `PKG_CHECK_MODULES `. >>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>> OK, will fix. >>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>> I'm usually using multiple different NICs, so I find this >>>>>>>>>>>>>>> information useful for logging test runs. Perhaps use >>>>>>>>>>>>>>> ODP_PRINT() here? >>>>>>>>>>>>>>>> Matias Elo(matiaselo) wrote: >>>>>>>>>>>>>>>> Will fix. >>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>> Just use `PKG_CHECK_MODULES` here rather than inventing >>>>>>>>>>>>>>>>> variables and checks. >>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>> Ideally we do not need this field. We should always return >>>>>>>>>>>>>>>>>> valid setting. Maybe by allowing ODP modules to return >>>>>>>>>>>>>>>>>> default value. >>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>> I'd prefer not to export config internals here, but rather >>>>>>>>>>>>>>>>>>> have `_odp_config_lookup_int()`, >>>>>>>>>>>>>>>>>>> `_odp_config_lookup_string()`, etc. >>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>> Last line should not be necessary >>>>>>>>>>>>>>>>>>>>> bogdanPricope wrote >>>>>>>>>>>>>>>>>>>>> OFP is using a similar naming for the environment >>>>>>>>>>>>>>>>>>>>> variable but with 'CONF' instead of 'CONFIG' >>>>>>>>>>>>>>>>>>>>> ('OFP_CONF_FILE'). Will it makes sense to change the >>>>>>>>>>>>>>>>>>>>> naming to avoid confusions: either 'ODP_CONF_FILE' in ODP >>>>>>>>>>>>>>>>>>>>> or 'OFP_CONFIG_FILE' in OFP? >>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>> Do we really want to print this unconditionally? In any >>>>>>>>>>>>>>>>>>>>>> event shouldn't this be `ODP_LOG()` here rather than >>>>>>>>>>>>>>>>>>>>>> `printf()`? >>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>> According to the [libconfig >>>>>>>>>>>>>>>>>>>>>>> changelog](https://github.com/hyperrealm/libconfig/blob/master/ChangeLog) >>>>>>>>>>>>>>>>>>>>>>> there are versions pre-1.0 (e.g., 0.9) which would >>>>>>>>>>>>>>>>>>>>>>> fail this test. This needs to be reversed so that you >>>>>>>>>>>>>>>>>>>>>>> use the newer form for v1.5 and higher levels: >>>>>>>>>>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>>>>>>>>>> #if (LIBCONFIG_VER_MAJOR > 1 || (LIBCONFIG_VER_MAJOR == >>>>>>>>>>>>>>>>>>>>>>> 1 && LIBCONFIG_VER_MINOR >= 5)) ... >>>>>>>>>>>>>>>>>>>>>>> ``` >>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>>> Does this have to be a hard dependency? Can we have >>>>>>>>>>>>>>>>>>>>>>>> this feature be omitted (hardcoded defaults are used) >>>>>>>>>>>>>>>>>>>>>>>> if libconfig is not available? https://github.com/Linaro/odp/pull/499#discussion_r171261100 updated_at 2018-02-28 14:40:20