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

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

On Tue, Feb 23, 2021 at 03:01:12PM -0800, Jie Zhou wrote:
> From: Jie Zhou <jizh@microsoft.com>
> 
> This patch allows the same set of rte_metrics_tel_* functions to be
> exported no matter JANSSON is available or not, by doing following:
> 1.	Leverage dpdk_conf to set configuration flag RTE_HAVE_JANSSON
> when Jansson dependency is found.
> 2.	In rte_metrics_telemetry.c, leverage RTE_HAVE_JANSSON to handle the
> case when JANSSON is not available by adding stubs for all the instances.
> 3.	In meson.build, per dpdk\doc\guides\rel_notes\release_20_05.rst,
> it is claimed that "Telemetry library is no longer dependent on the
> external Jansson library, which allows Telemetry be enabled by default.",
> thus make the deps and includes of Telemetry as not conditional anymore.
> 
> V2 changes:
>     Address comments from Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> and
>     Bruce Richardson <bruce.richardson@intel.com>:
>         - Set dpdk.conf RTE_HAS_JANSSON in metrics meson.build
>         - Reduce #ifdef RTE_HAS_JANSSON blocks
> 
> Signed-off-by: Jie Zhou <jizh@microsoft.com>

One small nit below, but otherwise this looks better.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  lib/librte_metrics/meson.build             | 12 ++---
>  lib/librte_metrics/rte_metrics_telemetry.c | 60 +++++++++++++++++++++-
>  lib/librte_metrics/rte_metrics_telemetry.h |  2 +-
>  3 files changed, 65 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_metrics/meson.build b/lib/librte_metrics/meson.build
> index 28a8cc115..e543e4cdd 100644
> --- a/lib/librte_metrics/meson.build
> +++ b/lib/librte_metrics/meson.build
> @@ -1,14 +1,14 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2017 Intel Corporation
>  
> -sources = files('rte_metrics.c')
> -headers = files('rte_metrics.h')
> +sources = files('rte_metrics.c', 'rte_metrics_telemetry.c')
> +headers = files('rte_metrics.h', 'rte_metrics_telemetry.h')
>  
>  jansson = dependency('jansson', required: false, method: 'pkg-config')
>  if jansson.found()
> +	dpdk_conf.set('RTE_HAS_JANSSON', 1)
>  	ext_deps += jansson
> -	sources += files('rte_metrics_telemetry.c')
> -	headers += files('rte_metrics_telemetry.h')
> -	deps += ['ethdev', 'telemetry']
> -	includes += include_directories('../librte_telemetry')
>  endif
> +
> +deps += ['ethdev', 'telemetry']
> +includes += include_directories('../librte_telemetry')

This includes line should not be necessary because the dependency implies
that the relevant header paths are added.

> diff --git a/lib/librte_metrics/rte_metrics_telemetry.c b/lib/librte_metrics/rte_metrics_telemetry.c
> index b8ee56ef0..305ee271f 100644
> --- a/lib/librte_metrics/rte_metrics_telemetry.c
> +++ b/lib/librte_metrics/rte_metrics_telemetry.c
> @@ -2,8 +2,6 @@
>   * Copyright(c) 2020 Intel Corporation
>   */
>  
> -#include <jansson.h>
> -
>  #include <rte_ethdev.h>
>  #include <rte_string_fns.h>
>  #ifdef RTE_LIB_TELEMETRY
> @@ -13,6 +11,7 @@
>  #include "rte_metrics.h"
>  #include "rte_metrics_telemetry.h"
>  
> +#ifdef RTE_HAS_JANSSON
>  struct telemetry_metrics_data tel_met_data;
>  
>  int metrics_log_level;
> @@ -541,3 +540,60 @@ RTE_INIT(metrics_ctor)
>  			handle_ports_stats_values_by_name);
>  #endif
>  }
> +#else
> +int32_t
> +rte_metrics_tel_reg_all_ethdev(int *metrics_register_done, int *reg_index_list)
> +{
> +	RTE_SET_USED(metrics_register_done);
> +	RTE_SET_USED(reg_index_list);
> +
> +	return -ENOTSUP;
> +}
> +
> +int32_t
> +rte_metrics_tel_encode_json_format(struct telemetry_encode_param *ep,
> +	char **json_buffer)
> +{
> +	RTE_SET_USED(ep);
> +	RTE_SET_USED(json_buffer);
> +
> +	return -ENOTSUP;
> +}
> +
> +int32_t
> +rte_metrics_tel_get_ports_stats_json(struct telemetry_encode_param *ep,
> +	int *reg_index, char **json_buffer)
> +{
> +	RTE_SET_USED(ep);
> +	RTE_SET_USED(reg_index);
> +	RTE_SET_USED(json_buffer);
> +
> +	return -ENOTSUP;
> +}
> +
> +int32_t
> +rte_metrics_tel_get_port_stats_ids(struct telemetry_encode_param *ep)
> +{
> +	RTE_SET_USED(ep);
> +
> +	return -ENOTSUP;
> +}
> +
> +int32_t
> +rte_metrics_tel_extract_data(struct telemetry_encode_param *ep, json_t *data)
> +{
> +	RTE_SET_USED(ep);
> +	RTE_SET_USED(data);
> +
> +	return -ENOTSUP;
> +}
> +
> +int32_t
> +rte_metrics_tel_get_global_stats(struct telemetry_encode_param *ep)
> +{
> +	RTE_SET_USED(ep);
> +
> +	return -ENOTSUP;
> +}
> +
> +#endif
> diff --git a/lib/librte_metrics/rte_metrics_telemetry.h b/lib/librte_metrics/rte_metrics_telemetry.h
> index 5dbb32ca0..2b6eb1ccc 100644
> --- a/lib/librte_metrics/rte_metrics_telemetry.h
> +++ b/lib/librte_metrics/rte_metrics_telemetry.h
> @@ -2,7 +2,7 @@
>   * Copyright(c) 2020 Intel Corporation
>   */
>  
> -#ifdef RTE_LIB_TELEMETRY
> +#ifdef RTE_HAS_JANSSON
>  #include <jansson.h>
>  #else
>  #define json_t void *
> -- 
> 2.30.0.vfs.0.2
>