Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/include/odp_internal.h
line 13
@@ -55,10 +56,13 @@ struct odp_global_data_s {
odp_cpumask_t control_cpus;
odp_cpumask_t worker_cpus;
int num_cpus_installed;
+ config_t libconfig; /*< Runtime config using libconfig */
+ uint8_t libconfig_enabled; /*< Runtime config enabled */
Comment:
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_r170598625
updated_at 2018-02-26 13:59:03
