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
