Re: [PATCH] telemetry: support boolean type

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

On Wed, Oct 19, 2022 at 09:37:02AM +0200, David Marchand wrote:
> Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

This patch looks pretty good to me. Some very small comments inline below.
One thing I notice is that we are not supporting booleans except as part of
an array or dictionary. Is it likely that we will ever want to have a
telemetry command that just returns true/false alone? Don't see that being
necessary just yet, so:

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

>  app/test/test_telemetry_data.c | 88 +++++++++++++++++++++++++++++++++-
>  lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++
>  lib/telemetry/telemetry.c      | 24 +++++++++-
>  lib/telemetry/telemetry_data.c | 44 +++++++++++++++--
>  lib/telemetry/telemetry_data.h |  5 ++
>  lib/telemetry/telemetry_json.h | 34 +++++++++++++
>  lib/telemetry/version.map      |  5 ++
>  7 files changed, 228 insertions(+), 8 deletions(-)
> 

<snip>

> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index a0d21d6b7f..5d74212f17 100644
> --- a/lib/telemetry/rte_telemetry.h
> +++ b/lib/telemetry/rte_telemetry.h
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2018 Intel Corporation
>   */
>  
> +#include <stdbool.h>
>  #include <stdint.h>
>  
>  #include <rte_compat.h>
> @@ -46,6 +47,7 @@ enum rte_tel_value_type {
>  	RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
>  	RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
>  	RTE_TEL_CONTAINER, /** a container struct */
> +	RTE_TEL_BOOL_VAL,   /** a boolean value */
>  };
>  
>  /**
> @@ -155,6 +157,22 @@ int
>  rte_tel_data_add_array_container(struct rte_tel_data *d,
>  		struct rte_tel_data *val, int keep);
>  
> +/**
> + * Add a boolean to an array.
> + * The array must have been started by rte_tel_data_start_array() with
> + * RTE_TEL_BOOL_VAL as the type parameter.
> + *
> + * @param d
> + *   The data structure passed to the callback
> + * @param x
> + *   The number to be returned in the array

number -> boolean value

> + * @return
> + *   0 on success, negative errno on error
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
> +
>  /**
>   * Add a string value to a dictionary.
>   * The dict must have been started by rte_tel_data_start_dict().
> @@ -233,6 +251,24 @@ int
>  rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
>  		struct rte_tel_data *val, int keep);
>  
> +/**
> + * Add a boolean value to a dictionary.
> + * The dict must have been started by rte_tel_data_start_dict().
> + *
> + * @param d
> + *   The data structure passed to the callback
> + * @param name
> + *   The name the value is to be stored under in the dict
> + *   Must contain only alphanumeric characters or the symbols: '_' or '/'
> + * @param val
> + *   The number to be stored in the dict

number -> boolean value

> + * @return
> + *   0 on success, negative errno on error, E2BIG on string truncation of name.
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_dict_bool(struct rte_tel_data *d, const char *name, bool val);
> +
>  /**
>   * This telemetry callback is used when registering a telemetry command.
>   * It handles getting and formatting information to be returned to telemetry
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
<snip>
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 5b319c18fb..4f81f71e03 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -16,10 +16,11 @@ int
>  rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
>  {
>  	enum tel_container_types array_types[] = {
> -			RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> -			RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> -			RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
> -			RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> +		[RTE_TEL_STRING_VAL] = RTE_TEL_ARRAY_STRING,
> +		[RTE_TEL_INT_VAL] = RTE_TEL_ARRAY_INT,
> +		[RTE_TEL_U64_VAL] = RTE_TEL_ARRAY_U64,
> +		[RTE_TEL_CONTAINER] = RTE_TEL_ARRAY_CONTAINER,
> +		[RTE_TEL_BOOL_VAL] = RTE_TEL_ARRAY_BOOL,
>  	};

Really like this change!

>  	d->type = array_types[type];
>  	d->data_len = 0;
> @@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
>  	return 0;
>  }
>  
> +int
> +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x)
> +{
> +	if (d->type != RTE_TEL_ARRAY_BOOL)
> +		return -EINVAL;
> +	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> +		return -ENOSPC;
> +	d->data.array[d->data_len++].boolval = x;
> +	return 0;
> +}
> +
>  int
>  rte_tel_data_add_array_container(struct rte_tel_data *d,
>  		struct rte_tel_data *val, int keep)
<snip>
> diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> index e3fae7c30d..c97da97366 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -7,6 +7,7 @@
>  
>  #include <inttypes.h>
>  #include <stdarg.h>
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <rte_common.h>
>  #include <rte_telemetry.h>
> @@ -159,6 +160,21 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
>  	return ret == 0 ? used : end + ret;
>  }
>  
> +/* Appends a boolean into the JSON array in the provided buffer. */
> +static inline int
> +rte_tel_json_add_array_bool(char *buf, const int len, const int used,
> +		bool val)
> +{
> +	int ret, end = used - 1; /* strip off final delimiter */
> +	if (used <= 2) /* assume empty, since minimum is '[]' */
> +		return __json_snprintf(buf, len, "[%s]",
> +				val ? "true" : "false");
> +
> +	ret = __json_snprintf(buf + end, len - end, ",%s]",
> +			val ? "true" : "false");

Wonder if it's worthwhile doing a macro for this conditional, since the
same ternary-operator snippet appears 4 times in this code.

> +	return ret == 0 ? used : end + ret;
> +}
> +
>  /*
>   * Add a new element with raw JSON value to the JSON array stored in the
>   * provided buffer.
> @@ -193,6 +209,24 @@ rte_tel_json_add_obj_u64(char *buf, const int len, const int used,
>  	return ret == 0 ? used : end + ret;
>  }
>  
<snip>