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

Reply via email to