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

[Bruce Richardson <bruce.richardson@intel.com>]

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:
> [...]
> > 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?
> 
> [...]
> > --- 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.
> 
> [...]
> >  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