Dmitry Eremin-Solenikov(lumag) replied on github web page: platform/linux-generic/include/odp_internal.h line 15 @@ -55,10 +56,16 @@ struct odp_global_data_s { odp_cpumask_t control_cpus; odp_cpumask_t worker_cpus; int num_cpus_installed; + config_t *libconfig_default; + config_t *libconfig_runtime; + config_t libconfig_default_structure; + config_t libconfig_runtime_structure;
Comment: Merging is not required in the first pass, but it might be a good thing in future. > Matias Elo(matiaselo) wrote: > OK >> Matias Elo(matiaselo) wrote: >> OK >>> Matias Elo(matiaselo) wrote: >>> OK >>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>> Last line should go to `nodist_noinst_HEADERS`, so it does not get >>>> included into distribution. >>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>> Just drop this setting/check. `PKG_CHECK_MODULES` will print usefull >>>>> message instead. >>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>> Should ODP install this header file somewhere? >>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>> 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_r171282910 updated_at 2018-02-28 15:32:04