Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/Makefile.am
line 14
@@ -94,6 +96,8 @@ noinst_HEADERS = \
include/odp_forward_typedefs_internal.h \
include/odp_internal.h \
include/odp_ipsec_internal.h \
+ include/odp_libconfig_config.h \
+ include/odp_libconfig_internal.h \
Comment:
Adding `DISTCLEANFILES = include/odp_libconfig_config.h` fixed the discheck
failure when odp_libconfig_config.h is added to noinst_HEADERS.
> Matias Elo(matiaselo) wrote:
> I assume you meant odp_libconfig_config.h Anyway, doing this breaks
> distcheck build:
>
> ```
> CC odp_libconfig.lo
> ../../../../platform/linux-generic/odp_libconfig.c:17:34: fatal error:
> odp_libconfig_config.h: No such file or directory
> compilation terminated.
> Makefile:1120: recipe for target 'odp_libconfig.lo' failed
> make[2]: *** [odp_libconfig.lo] Error 1
> make[2]: Leaving directory
> '/home/nnn/dev/odp.git/opendataplane-1.17.0.0/_build/sub/platform/linux-generic'
> ```
>
> With both headers in noinst_HEADERS distcheck completes but complains at the
> end:
> ```
> ERROR: files left in build directory after distclean:
> ./platform/linux-generic/include/odp_libconfig_config.h
> Makefile:793: recipe for target 'distcleancheck' failed
> make[1]: *** [distcleancheck] Error 1
> make[1]: Leaving directory
> '/home/nnn/dev/odp.git/opendataplane-1.17.0.0/_build/sub'
> Makefile:719: recipe for target 'distcheck' failed
> make: *** [distcheck] Error 1
> ```
>
> Any pointers?
>> Matias Elo(matiaselo) wrote:
>> A good point. Would this be OK?
>>
>> .../platform/Makefile.inc
>> ```
>> configdir = $(sysconfdir)/odp
>> config_DATA = $(top_srcdir)/config/odp-$(with_platform).conf
>>
>> ```
>>> Matias Elo(matiaselo) wrote:
>>> Thanks! I'll try this.
Dmitry Eremin-Solenikov(lumag) wrote:
Typical idiom is
```make
include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
$(top_builddir)/config.status
cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
```
I'd suggest you to try building with separate builddir, it should help you
to catch build/src dir differences.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Makefile part:
> ```make
> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
> $(top_srcdir)/config.status
> cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
> ```
>
> Also it might be nice to use
> platform/${with_platform}/odp_libconfig_config.h as tag name, so that
> tags for different platforms won't collide.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> I'd say
>> ```m4
>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>> [ mkdir -p platform/${with_platform}/include
>> xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
>> 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>platform/${with_platform}/include/odp_libconfig_config.h])
>> ```
>>> Matias Elo(matiaselo) wrote:
>>> I need some assistance with this. What I have now is:
>>>
>>> ```
>>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>>> [
>>> pushd ${ac_abs_top_builddir}/config
>>> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>>../platform/${with_platform}/include/odp_libconfig_config.h
>>> popd],
>>> [with_platform=$with_platform]
>>> )
>>> ```
>>>
>>> And in .../linux-generic/Makefile.am:
>>> ```
>>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>>> $(top_srcdir)/config.status
>>> ./$(top_srcdir)/config.status odp_libconfig_config.h
>>> ```
>>> This seems to work as it detects changes in odp-linux.conf, but I'm not
>>> sure if this is the proper way to do this.
Dmitry Eremin-Solenikov(lumag) wrote:
Builddir is current directory.
> Matias Elo(matiaselo) wrote:
> builddir is not defined here. Any ideas?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Consider somebody changing conf file and reruning make. Make should
>> detect that header should be regenerated and invoke ./config.status
>> with proper tag to regenrate header in question.
>>> Matias Elo(matiaselo) wrote:
>>> > Also there should be a dependency line in respective Makefile.am
>>> > to regenerate the header.
>>>
>>> Can you elaborate this a bit?
Matias Elo(matiaselo) wrote:
Yeah, I actually tried merging the files but it ended up being
painful since libconfig doesn't provide good functions for doing
that. So I went with the "safe" solution. I'll try if I can at
least get rid of the extra pointers.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Merging is not
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/Makefile.am
line 14
@@ -94,6 +96,8 @@ noinst_HEADERS = \
include/odp_forward_typedefs_internal.h \
include/odp_internal.h \
include/odp_ipsec_internal.h \
+ include/odp_libconfig_config.h \
+ include/odp_libconfig_internal.h \
Comment:
I assume you meant odp_libconfig_config.h Anyway, doing this breaks distcheck
build:
```
CC odp_libconfig.lo
../../../../platform/linux-generic/odp_libconfig.c:17:34: fatal error:
odp_libconfig_config.h: No such file or directory
compilation terminated.
Makefile:1120: recipe for target 'odp_libconfig.lo' failed
make[2]: *** [odp_libconfig.lo] Error 1
make[2]: Leaving directory
'/home/nnn/dev/odp.git/opendataplane-1.17.0.0/_build/sub/platform/linux-generic'
```
With both headers in noinst_HEADERS distcheck completes but complains at the
end:
```
ERROR: files left in build directory after distclean:
./platform/linux-generic/include/odp_libconfig_config.h
Makefile:793: recipe for target 'distcleancheck' failed
make[1]: *** [distcleancheck] Error 1
make[1]: Leaving directory
'/home/nnn/dev/odp.git/opendataplane-1.17.0.0/_build/sub'
Makefile:719: recipe for target 'distcheck' failed
make: *** [distcheck] Error 1
```
Any pointers?
> Matias Elo(matiaselo) wrote:
> A good point. Would this be OK?
>
> .../platform/Makefile.inc
> ```
> configdir = $(sysconfdir)/odp
> config_DATA = $(top_srcdir)/config/odp-$(with_platform).conf
>
> ```
>> Matias Elo(matiaselo) wrote:
>> Thanks! I'll try this.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Typical idiom is
>>> ```make
>>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>>> $(top_builddir)/config.status
>>> cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
>>> ```
>>>
>>> I'd suggest you to try building with separate builddir, it should help you
>>> to catch build/src dir differences.
Dmitry Eremin-Solenikov(lumag) wrote:
Makefile part:
```make
include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
$(top_srcdir)/config.status
cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
```
Also it might be nice to use
platform/${with_platform}/odp_libconfig_config.h as tag name, so that tags
for different platforms won't collide.
> Dmitry Eremin-Solenikov(lumag) wrote:
> I'd say
> ```m4
> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
> [ mkdir -p platform/${with_platform}/include
> xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
> 's/\([0-9a-f]\)$/\0, 0x00/' > \
>platform/${with_platform}/include/odp_libconfig_config.h])
> ```
>> Matias Elo(matiaselo) wrote:
>> I need some assistance with this. What I have now is:
>>
>> ```
>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>> [
>> pushd ${ac_abs_top_builddir}/config
>> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>../platform/${with_platform}/include/odp_libconfig_config.h
>> popd],
>> [with_platform=$with_platform]
>> )
>> ```
>>
>> And in .../linux-generic/Makefile.am:
>> ```
>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>> $(top_srcdir)/config.status
>> ./$(top_srcdir)/config.status odp_libconfig_config.h
>> ```
>> This seems to work as it detects changes in odp-linux.conf, but I'm not
>> sure if this is the proper way to do this.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Builddir is current directory.
Matias Elo(matiaselo) wrote:
builddir is not defined here. Any ideas?
> Dmitry Eremin-Solenikov(lumag) wrote:
> Consider somebody changing conf file and reruning make. Make should
> detect that header should be regenerated and invoke ./config.status
> with proper tag to regenrate header in question.
>> Matias Elo(matiaselo) wrote:
>> > Also there should be a dependency line in respective Makefile.am
>> > to regenerate the header.
>>
>> Can you elaborate this a bit?
>>> Matias Elo(matiaselo) wrote:
>>> Yeah, I actually tried merging the files but it ended up being
>>> painful since libconfig doesn't provide good functions for doing
>>> that. So I went with the "safe" solution. I'll try if I can at
>>> least get rid of the extra pointers.
Dmitry Eremin-Solenikov(lumag) wrote:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
Makefile.am
line 5
@@ -20,7 +20,7 @@ SUBDIRS = \
@DX_RULES@
-EXTRA_DIST = bootstrap CHANGELOG config/README
+EXTRA_DIST = bootstrap CHANGELOG config/README config/odp-linux.conf
Comment:
A good point. Would this be OK?
.../platform/Makefile.inc
```
configdir = $(sysconfdir)/odp
config_DATA = $(top_srcdir)/config/odp-$(with_platform).conf
```
> Matias Elo(matiaselo) wrote:
> Thanks! I'll try this.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Typical idiom is
>> ```make
>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>> $(top_builddir)/config.status
>> cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
>> ```
>>
>> I'd suggest you to try building with separate builddir, it should help you
>> to catch build/src dir differences.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Makefile part:
>>> ```make
>>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>>> $(top_srcdir)/config.status
>>> cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
>>> ```
>>>
>>> Also it might be nice to use
>>> platform/${with_platform}/odp_libconfig_config.h as tag name, so that tags
>>> for different platforms won't collide.
Dmitry Eremin-Solenikov(lumag) wrote:
I'd say
```m4
AC_CONFIG_COMMANDS([odp_libconfig_config.h],
[ mkdir -p platform/${with_platform}/include
xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
's/\([0-9a-f]\)$/\0, 0x00/' > \
platform/${with_platform}/include/odp_libconfig_config.h])
```
> Matias Elo(matiaselo) wrote:
> I need some assistance with this. What I have now is:
>
> ```
> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
> [
> pushd ${ac_abs_top_builddir}/config
> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>../platform/${with_platform}/include/odp_libconfig_config.h
> popd],
> [with_platform=$with_platform]
> )
> ```
>
> And in .../linux-generic/Makefile.am:
> ```
> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
> $(top_srcdir)/config.status
> ./$(top_srcdir)/config.status odp_libconfig_config.h
> ```
> This seems to work as it detects changes in odp-linux.conf, but I'm not
> sure if this is the proper way to do this.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Builddir is current directory.
>>> Matias Elo(matiaselo) wrote:
>>> builddir is not defined here. Any ideas?
Dmitry Eremin-Solenikov(lumag) wrote:
Consider somebody changing conf file and reruning make. Make should
detect that header should be regenerated and invoke ./config.status
with proper tag to regenrate header in question.
> Matias Elo(matiaselo) wrote:
> > Also there should be a dependency line in respective Makefile.am to
> > regenerate the header.
>
> Can you elaborate this a bit?
>> Matias Elo(matiaselo) wrote:
>> Yeah, I actually tried merging the files but it ended up being
>> painful since libconfig doesn't provide good functions for doing
>> that. So I went with the "safe" solution. I'll try if I can at least
>> get rid of the extra pointers.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> 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)
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 31
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
+ AC_SUBST([LIBCONFIG_CFLAGS])
+ AC_SUBST([LIBCONFIG_LIBS])
Comment:
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.
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
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
+#include
+
+#include
+#include
+#include
+#include
+
+extern struct odp_global_data_s odp_global_data;
+
+int _odp_libconfig_init_global(void)
+{
+ char *filename;
+ config_t *config = _global_data.libconfig_default_structure;
+ config_t *config_rt = _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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
Thanks! I'll try this.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Typical idiom is
> ```make
> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
> $(top_builddir)/config.status
> cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
> ```
>
> I'd suggest you to try building with separate builddir, it should help you to
> catch build/src dir differences.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Makefile part:
>> ```make
>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>> $(top_srcdir)/config.status
>> cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
>> ```
>>
>> Also it might be nice to use
>> platform/${with_platform}/odp_libconfig_config.h as tag name, so that tags
>> for different platforms won't collide.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> I'd say
>>> ```m4
>>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>>> [ mkdir -p platform/${with_platform}/include
>>> xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
>>> 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>>platform/${with_platform}/include/odp_libconfig_config.h])
>>> ```
Matias Elo(matiaselo) wrote:
I need some assistance with this. What I have now is:
```
AC_CONFIG_COMMANDS([odp_libconfig_config.h],
[
pushd ${ac_abs_top_builddir}/config
xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
../platform/${with_platform}/include/odp_libconfig_config.h
popd],
[with_platform=$with_platform]
)
```
And in .../linux-generic/Makefile.am:
```
include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
$(top_srcdir)/config.status
./$(top_srcdir)/config.status odp_libconfig_config.h
```
This seems to work as it detects changes in odp-linux.conf, but I'm not
sure if this is the proper way to do this.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Builddir is current directory.
>> Matias Elo(matiaselo) wrote:
>> builddir is not defined here. Any ideas?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Consider somebody changing conf file and reruning make. Make should
>>> detect that header should be regenerated and invoke ./config.status
>>> with proper tag to regenrate header in question.
Matias Elo(matiaselo) wrote:
> Also there should be a dependency line in respective Makefile.am to
> regenerate the header.
Can you elaborate this a bit?
> Matias Elo(matiaselo) wrote:
> Yeah, I actually tried merging the files but it ended up being
> painful since libconfig doesn't provide good functions for doing
> that. So I went with the "safe" solution. I'll try if I can at least
> get rid of the extra pointers.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
Typical idiom is
```make
include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
$(top_builddir)/config.status
cd $(top_builddir) && $(SHELL) ./config.status $(subdir)/$@
```
I'd suggest you to try building with separate builddir, it should help you to
catch build/src dir differences.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Makefile part:
> ```make
> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
> $(top_srcdir)/config.status
> cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
> ```
>
> Also it might be nice to use platform/${with_platform}/odp_libconfig_config.h
> as tag name, so that tags for different platforms won't collide.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> I'd say
>> ```m4
>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>> [ mkdir -p platform/${with_platform}/include
>> xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
>> 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>platform/${with_platform}/include/odp_libconfig_config.h])
>> ```
>>> Matias Elo(matiaselo) wrote:
>>> I need some assistance with this. What I have now is:
>>>
>>> ```
>>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>>> [
>>> pushd ${ac_abs_top_builddir}/config
>>> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>>../platform/${with_platform}/include/odp_libconfig_config.h
>>> popd],
>>> [with_platform=$with_platform]
>>> )
>>> ```
>>>
>>> And in .../linux-generic/Makefile.am:
>>> ```
>>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>>> $(top_srcdir)/config.status
>>> ./$(top_srcdir)/config.status odp_libconfig_config.h
>>> ```
>>> This seems to work as it detects changes in odp-linux.conf, but I'm not
>>> sure if this is the proper way to do this.
Dmitry Eremin-Solenikov(lumag) wrote:
Builddir is current directory.
> Matias Elo(matiaselo) wrote:
> builddir is not defined here. Any ideas?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Consider somebody changing conf file and reruning make. Make should
>> detect that header should be regenerated and invoke ./config.status with
>> proper tag to regenrate header in question.
>>> Matias Elo(matiaselo) wrote:
>>> > Also there should be a dependency line in respective Makefile.am to
>>> > regenerate the header.
>>>
>>> Can you elaborate this a bit?
Matias Elo(matiaselo) wrote:
Yeah, I actually tried merging the files but it ended up being painful
since libconfig doesn't provide good functions for doing that. So I
went with the "safe" solution. I'll try if I can at least get rid of
the extra pointers.
> Dmitry Eremin-Solenikov(lumag) wrote:
> 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?
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
Makefile part:
```make
include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
$(top_srcdir)/config.status
cd $(top_builddir) && $(SHELL) ./config.status odp_libconfig_config.h
```
Also it might be nice to use platform/${with_platform}/odp_libconfig_config.h
as tag name, so that tags for different platforms won't collide.
> Dmitry Eremin-Solenikov(lumag) wrote:
> I'd say
> ```m4
> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
> [ mkdir -p platform/${with_platform}/include
> xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
> 's/\([0-9a-f]\)$/\0, 0x00/' > \
>platform/${with_platform}/include/odp_libconfig_config.h])
> ```
>> Matias Elo(matiaselo) wrote:
>> I need some assistance with this. What I have now is:
>>
>> ```
>> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
>> [
>> pushd ${ac_abs_top_builddir}/config
>> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>>../platform/${with_platform}/include/odp_libconfig_config.h
>> popd],
>> [with_platform=$with_platform]
>> )
>> ```
>>
>> And in .../linux-generic/Makefile.am:
>> ```
>> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
>> $(top_srcdir)/config.status
>> ./$(top_srcdir)/config.status odp_libconfig_config.h
>> ```
>> This seems to work as it detects changes in odp-linux.conf, but I'm not sure
>> if this is the proper way to do this.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Builddir is current directory.
Matias Elo(matiaselo) wrote:
builddir is not defined here. Any ideas?
> Dmitry Eremin-Solenikov(lumag) wrote:
> Consider somebody changing conf file and reruning make. Make should
> detect that header should be regenerated and invoke ./config.status with
> proper tag to regenrate header in question.
>> Matias Elo(matiaselo) wrote:
>> > Also there should be a dependency line in respective Makefile.am to
>> > regenerate the header.
>>
>> Can you elaborate this a bit?
>>> Matias Elo(matiaselo) wrote:
>>> Yeah, I actually tried merging the files but it ended up being painful
>>> since libconfig doesn't provide good functions for doing that. So I
>>> went with the "safe" solution. I'll try if I can at least get rid of
>>> the extra pointers.
Dmitry Eremin-Solenikov(lumag) wrote:
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.
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 19
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
I'd say
```m4
AC_CONFIG_COMMANDS([odp_libconfig_config.h],
[ mkdir -p platform/${with_platform}/include
xxd -i ${srcdir}/platform/${with_platform}/odp-linux.conf | sed
's/\([0-9a-f]\)$/\0, 0x00/' > \
platform/${with_platform}/include/odp_libconfig_config.h])
```
> Matias Elo(matiaselo) wrote:
> I need some assistance with this. What I have now is:
>
> ```
> AC_CONFIG_COMMANDS([odp_libconfig_config.h],
> [
> pushd ${ac_abs_top_builddir}/config
> xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
>../platform/${with_platform}/include/odp_libconfig_config.h
> popd],
> [with_platform=$with_platform]
> )
> ```
>
> And in .../linux-generic/Makefile.am:
> ```
> include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
> $(top_srcdir)/config.status
> ./$(top_srcdir)/config.status odp_libconfig_config.h
> ```
> This seems to work as it detects changes in odp-linux.conf, but I'm not sure
> if this is the proper way to do this.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Builddir is current directory.
>>> Matias Elo(matiaselo) wrote:
>>> builddir is not defined here. Any ideas?
Dmitry Eremin-Solenikov(lumag) wrote:
Consider somebody changing conf file and reruning make. Make should detect
that header should be regenerated and invoke ./config.status with proper
tag to regenrate header in question.
> Matias Elo(matiaselo) wrote:
> > Also there should be a dependency line in respective Makefile.am to
> > regenerate the header.
>
> Can you elaborate this a bit?
>> Matias Elo(matiaselo) wrote:
>> Yeah, I actually tried merging the files but it ended up being painful
>> since libconfig doesn't provide good functions for doing that. So I went
>> with the "safe" solution. I'll try if I can at least get rid of the
>> extra pointers.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
I need some assistance with this. What I have now is:
```
AC_CONFIG_COMMANDS([odp_libconfig_config.h],
[
pushd ${ac_abs_top_builddir}/config
xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
../platform/${with_platform}/include/odp_libconfig_config.h
popd],
[with_platform=$with_platform]
)
```
And in .../linux-generic/Makefile.am:
```
include/odp_libconfig_config.h: $(top_srcdir)/config/odp-linux.conf
$(top_srcdir)/config.status
./$(top_srcdir)/config.status odp_libconfig_config.h
```
This seems to work as it detects changes in odp-linux.conf, but I'm not sure if
this is the proper way to do this.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Builddir is current directory.
>> Matias Elo(matiaselo) wrote:
>> builddir is not defined here. Any ideas?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Consider somebody changing conf file and reruning make. Make should detect
>>> that header should be regenerated and invoke ./config.status with proper
>>> tag to regenrate header in question.
Matias Elo(matiaselo) wrote:
> Also there should be a dependency line in respective Makefile.am to
> regenerate the header.
Can you elaborate this a bit?
> Matias Elo(matiaselo) wrote:
> Yeah, I actually tried merging the files but it ended up being painful
> since libconfig doesn't provide good functions for doing that. So I went
> with the "safe" solution. I'll try if I can at least get rid of the extra
> pointers.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
builddir is not defined here. Any ideas?
> Dmitry Eremin-Solenikov(lumag) wrote:
> Consider somebody changing conf file and reruning make. Make should detect
> that header should be regenerated and invoke ./config.status with proper tag
> to regenrate header in question.
>> Matias Elo(matiaselo) wrote:
>> > Also there should be a dependency line in respective Makefile.am to
>> > regenerate the header.
>>
>> Can you elaborate this a bit?
>>> Matias Elo(matiaselo) wrote:
>>> Yeah, I actually tried merging the files but it ended up being painful
>>> since libconfig doesn't provide good functions for doing that. So I went
>>> with the "safe" solution. I'll try if I can at least get rid of the extra
>>> pointers.
Dmitry Eremin-Solenikov(lumag) wrote:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
Builddir is current directory.
> Matias Elo(matiaselo) wrote:
> builddir is not defined here. Any ideas?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Consider somebody changing conf file and reruning make. Make should detect
>> that header should be regenerated and invoke ./config.status with proper tag
>> to regenrate header in question.
>>> Matias Elo(matiaselo) wrote:
>>> > Also there should be a dependency line in respective Makefile.am to
>>> > regenerate the header.
>>>
>>> Can you elaborate this a bit?
Matias Elo(matiaselo) wrote:
Yeah, I actually tried merging the files but it ended up being painful
since libconfig doesn't provide good functions for doing that. So I went
with the "safe" solution. I'll try if I can at least get rid of the extra
pointers.
> Dmitry Eremin-Solenikov(lumag) wrote:
> 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.
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 29
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
Comment:
> Also there should be a dependency line in respective Makefile.am to
> regenerate the header.
Can you elaborate this a bit?
> Matias Elo(matiaselo) wrote:
> Yeah, I actually tried merging the files but it ended up being painful since
> libconfig doesn't provide good functions for doing that. So I went with the
> "safe" solution. I'll try if I can at least get rid of the extra pointers.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 29
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
Comment:
Consider somebody changing conf file and reruning make. Make should detect that
header should be regenerated and invoke ./config.status with proper tag to
regenrate header in question.
> Matias Elo(matiaselo) wrote:
> > Also there should be a dependency line in respective Makefile.am to
> > regenerate the header.
>
> Can you elaborate this a bit?
>> Matias Elo(matiaselo) wrote:
>> Yeah, I actually tried merging the files but it ended up being painful since
>> libconfig doesn't provide good functions for doing that. So I went with the
>> "safe" solution. I'll try if I can at least get rid of the extra pointers.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) 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:
Yeah, I actually tried merging the files but it ended up being painful since
libconfig doesn't provide good functions for doing that. So I went with the
"safe" solution. I'll try if I can at least get rid of the extra pointers.
> Dmitry Eremin-Solenikov(lumag) wrote:
> 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
>>>
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/Makefile.am
line 14
@@ -94,6 +96,8 @@ noinst_HEADERS = \
include/odp_forward_typedefs_internal.h \
include/odp_internal.h \
include/odp_ipsec_internal.h \
+ include/odp_libconfig_config.h \
+ include/odp_libconfig_internal.h \
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 11
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
Makefile.am
line 5
@@ -20,7 +20,7 @@ SUBDIRS = \
@DX_RULES@
-EXTRA_DIST = bootstrap CHANGELOG config/README
+EXTRA_DIST = bootstrap CHANGELOG config/README config/odp-linux.conf
Comment:
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:
>>>
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/Makefile.am
line 14
@@ -94,6 +96,8 @@ noinst_HEADERS = \
include/odp_forward_typedefs_internal.h \
include/odp_internal.h \
include/odp_ipsec_internal.h \
+ include/odp_libconfig_config.h \
+ include/odp_libconfig_internal.h \
Comment:
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:
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 31
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
+ AC_SUBST([LIBCONFIG_CFLAGS])
+ AC_SUBST([LIBCONFIG_LIBS])
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/libodp-linux.pc.in
line 5
@@ -7,5 +7,5 @@ Name: libodp-linux
Description: The ODP packet processing engine
Version: @PKGCONFIG_VERSION@
Libs: -L${libdir} -lodp-linux
-Libs.private: @OPENSSL_STATIC_LIBS@ @DPDK_LIBS@ @PCAP_LIBS@ @PTHREAD_LIBS@
@TIMER_LIBS@ -lpthread @ATOMIC_LIBS@
+Libs.private: @OPENSSL_STATIC_LIBS@ @LIBCONFIG_LIBS@ @DPDK_LIBS@ @PCAP_LIBS@
@PTHREAD_LIBS@ @TIMER_LIBS@ -lpthread @ATOMIC_LIBS@
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
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
>>>
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
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:
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?
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 29
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 11
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 29
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
Comment:
Consider somebody changing conf file and reruning make. Make should detect that
header should be regenerated and invoke ./config.status with proper tag to
regenrate header in question.
> Matias Elo(matiaselo) wrote:
> > Also there should be a dependency line in respective Makefile.am to
> > regenerate the header.
>
> Can you elaborate this a bit?
>> Matias Elo(matiaselo) wrote:
>> Yeah, I actually tried merging the files but it ended up being painful since
>> libconfig doesn't provide good functions for doing that. So I went with the
>> "safe" solution. I'll try if I can at least get rid of the extra pointers.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 29
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
Comment:
> Also there should be a dependency line in respective Makefile.am to
> regenerate the header.
Can you elaborate this a bit?
> Matias Elo(matiaselo) wrote:
> Yeah, I actually tried merging the files but it ended up being painful since
> libconfig doesn't provide good functions for doing that. So I went with the
> "safe" solution. I'll try if I can at least get rid of the extra pointers.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> 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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) 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:
Yeah, I actually tried merging the files but it ended up being painful since
libconfig doesn't provide good functions for doing that. So I went with the
"safe" solution. I'll try if I can at least get rid of the extra pointers.
> Dmitry Eremin-Solenikov(lumag) wrote:
> 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
>>>
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
platform/linux-generic/Makefile.am
line 14
@@ -94,6 +96,8 @@ noinst_HEADERS = \
include/odp_forward_typedefs_internal.h \
include/odp_internal.h \
include/odp_ipsec_internal.h \
+ include/odp_libconfig_config.h \
+ include/odp_libconfig_internal.h \
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 31
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
+ AC_SUBST([LIBCONFIG_CFLAGS])
+ AC_SUBST([LIBCONFIG_LIBS])
Comment:
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.
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Matias Elo(matiaselo) replied on github web page:
m4/odp_libconfig.m4
line 11
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 19
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
Makefile.am
line 5
@@ -20,7 +20,7 @@ SUBDIRS = \
@DX_RULES@
-EXTRA_DIST = bootstrap CHANGELOG config/README
+EXTRA_DIST = bootstrap CHANGELOG config/README config/odp-linux.conf
Comment:
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:
>>>
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
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
+#include
+
+#include
+#include
+#include
+#include
+
+extern struct odp_global_data_s odp_global_data;
+
+int _odp_libconfig_init_global(void)
+{
+ char *filename;
+ config_t *config = _global_data.libconfig_default_structure;
+ config_t *config_rt = _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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/libodp-linux.pc.in
line 5
@@ -7,5 +7,5 @@ Name: libodp-linux
Description: The ODP packet processing engine
Version: @PKGCONFIG_VERSION@
Libs: -L${libdir} -lodp-linux
-Libs.private: @OPENSSL_STATIC_LIBS@ @DPDK_LIBS@ @PCAP_LIBS@ @PTHREAD_LIBS@
@TIMER_LIBS@ -lpthread @ATOMIC_LIBS@
+Libs.private: @OPENSSL_STATIC_LIBS@ @LIBCONFIG_LIBS@ @DPDK_LIBS@ @PCAP_LIBS@
@PTHREAD_LIBS@ @TIMER_LIBS@ -lpthread @ATOMIC_LIBS@
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 28
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
Comment:
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
>>>
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 29
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
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:
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?
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 11
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
Comment:
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
Re: [lng-odp] [PATCH v3] linux-gen: add runtime configuration file
Dmitry Eremin-Solenikov(lumag) replied on github web page:
m4/odp_libconfig.m4
line 31
@@ -0,0 +1,36 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+# -
+AC_DEFUN([ODP_LIBCONFIG],
+[dnl
+##
+# Set optional libconfig path
+##
+AC_ARG_WITH([libconfig-path],
+[AS_HELP_STRING([--with-libconfig-path=DIR],
+[path to libconfig libs and headers (use system path if not provided)])],
+[export PKG_CONFIG_PATH="$PKG_CONFIG_PATH:$withval/lib/pkgconfig"],
+)
+
+##
+# Check for libconfig availability
+##
+odp_libconfig_ok=yes
+PKG_CHECK_MODULES([LIBCONFIG], [libconfig], [],
+ [odp_libconfig_ok=no])
+
+if test "x$odp_libconfig_ok" = "xyes" ; then
+##
+# Create a header file odp_libconfig_config.h which containins null
+# terminated hex dump of odp-linux.conf
+##
+ [pushd ${srcdir}/config
+xxd -i odp-linux.conf | sed 's/\([0-9a-f]\)$/\0, 0x00/' > \
+ ../platform/${with_platform}/include/odp_libconfig_config.h
+popd]
+ AC_SUBST([LIBCONFIG_CFLAGS])
+ AC_SUBST([LIBCONFIG_LIBS])
Comment:
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
