Re: [dpdk-dev] [PATCH] rte_metrics: unconditionally export rte_metrics_tel_xxx functions

On Tue, Feb 23, 2021 at 01:24:15AM +0300, Dmitry Kozlyuk wrote:

+ Bruce

On Mon, 22 Feb 2021 13:25:02 -0800, Jie wrote:
[...]

quoted

diff --git a/config/meson.build b/config/meson.build
index 3cf560b8a..892bd9677 100644
--- a/config/meson.build
+++ b/config/meson.build

@@ -292,6 +292,11 @@ if is_freebsd
 	add_project_arguments('-D__BSD_VISIBLE', language: 'c')
 endif
 
+jansson = dependency('jansson', required: false, method: 'pkg-config')
+if jansson.found()
+	dpdk_conf.set('RTE_HAVE_JANSSON', 1)
+endif

DPDK seems to prefer "HAS" for such macros.

Why not do this in lib/librte_telemetry/meson.build?

[...]

quoted

--- a/lib/librte_metrics/meson.build
+++ b/lib/librte_metrics/meson.build

@@ -4,11 +4,12 @@
 sources = files('rte_metrics.c')
 headers = files('rte_metrics.h')
 
-jansson = dependency('jansson', required: false, method: 'pkg-config')
-if jansson.found()
+if dpdk_conf.has('RTE_HAVE_JANSSON')
 	ext_deps += jansson
-	sources += files('rte_metrics_telemetry.c')
-	headers += files('rte_metrics_telemetry.h')
-	deps += ['ethdev', 'telemetry']
-	includes += include_directories('../librte_telemetry')
 endif
+
+sources += files('rte_metrics_telemetry.c')
+headers += files('rte_metrics_telemetry.h')

Can be merged with definitions above.

[...]

quoted

 int32_t
 rte_metrics_tel_reg_all_ethdev(int *metrics_register_done, int *reg_index_list)
 {
+#ifdef JANSSON

Why not use RTE_HAS_JANSSON everywhere? (One more occurrence below.)

+1 for this suggestion.

Also, since this is essentially stubbing out the functions in this file,
can you reduce the amount of ifdefs by putting either at the start or the
end the full set of stubs, leaving just the one ifdef block covering the
whole file.

/Bruce