[v3,1/4] telemetry: support boolean type

Message ID 20221025090052.429232-2-david.marchand@redhat.com (mailing list archive)
State Deferred
Delegated to: David Marchand
Headers
Series Telemetry support for traces |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

David Marchand Oct. 25, 2022, 9 a.m. UTC
  Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
---
Changes since v1:
- fixed doxygen description,

---
 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      | 10 ++++
 7 files changed, 233 insertions(+), 8 deletions(-)
  

Comments

Mattias Rönnblom Oct. 25, 2022, 9:34 a.m. UTC | #1
On 2022-10-25 11:00, 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>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Ciara Power <ciara.power@intel.com>
> ---
> Changes since v1:
> - fixed doxygen description,
> 
> ---
>   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      | 10 ++++
>   7 files changed, 233 insertions(+), 8 deletions(-)
> 
> diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> index d92667a527..134e018fde 100644
> --- a/app/test/test_telemetry_data.c
> +++ b/app/test/test_telemetry_data.c
> @@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
>   	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
>   }
>   
> +static int
> +test_case_array_bool(void)
> +{
> +	int i;
> +
> +	rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
> +	for (i = 0; i < 5; i++)
> +		rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
> +	return CHECK_OUTPUT("[true,false,true,false,true]");
> +}
> +
> +static int
> +test_case_add_dict_bool(void)
> +{
> +	int i = 0;
> +	char name_of_value[8];
> +
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 5; i++) {
> +		sprintf(name_of_value, "dict_%d", i);
> +		rte_tel_data_add_dict_bool(&response_data, name_of_value,
> +			(i % 2) == 0);
> +	}
> +	return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
> +		"\"dict_3\":false,\"dict_4\":true}");
> +}
> +
> +static int
> +test_dict_with_array_bool_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> +
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 10; i++) {
> +		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> +		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> +	}
> +
> +	rte_tel_data_add_dict_container(&response_data, "dict_0",
> +	 child_data, 0);
> +	rte_tel_data_add_dict_container(&response_data, "dict_1",
> +	 child_data2, 0);
> +
> +	return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
> +		"\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
> +}
> +
> +static int
> +test_array_with_array_bool_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> +
> +	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> +
> +	for (i = 0; i < 5; i++) {
> +		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> +		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> +	}
> +	rte_tel_data_add_array_container(&response_data, child_data, 0);
> +	rte_tel_data_add_array_container(&response_data, child_data2, 0);
> +
> +	return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
> +}
> +
>   static int
>   test_string_char_escaping(void)
>   {
> @@ -428,15 +506,21 @@ telemetry_data_autotest(void)
>   			test_null_return,
>   			test_simple_string,
>   			test_case_array_string,
> -			test_case_array_int, test_case_array_u64,
> -			test_case_add_dict_int, test_case_add_dict_u64,
> +			test_case_array_int,
> +			test_case_array_u64,
> +			test_case_array_bool,
> +			test_case_add_dict_int,
> +			test_case_add_dict_u64,
> +			test_case_add_dict_bool,
>   			test_case_add_dict_string,
>   			test_dict_with_array_int_values,
>   			test_dict_with_array_u64_values,
> +			test_dict_with_array_bool_values,
>   			test_dict_with_array_string_values,
>   			test_dict_with_dict_values,
>   			test_array_with_array_int_values,
>   			test_array_with_array_u64_values,
> +			test_array_with_array_bool_values,
>   			test_array_with_array_string_values,
>   			test_string_char_escaping,
>   			test_array_char_escaping,
> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index a0d21d6b7f..e7f6c2ae43 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 boolean value to be returned in the array
> + * @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().

...as RTE_TEL_BOOL_VAL as the type parameter?

> + *
> + * @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 boolean value to be stored in the dict
> + * @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
> index 8fbb4f3060..276d0f337d 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>   	unsigned int i;
>   
>   	if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
> -		d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
> +			d->type != RTE_TEL_ARRAY_INT &&
> +			d->type != RTE_TEL_ARRAY_STRING &&
> +			d->type != RTE_TEL_ARRAY_BOOL)

Use a switch for improved readability.

>   		return snprintf(out_buf, buf_len, "null");
>   
>   	used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>   			used = rte_tel_json_add_array_string(out_buf,
>   				buf_len, used,
>   				d->data.array[i].sval);
> +	if (d->type == RTE_TEL_ARRAY_BOOL)

This whole section should be refactor to use a switch statement, imo.

> +		for (i = 0; i < d->data_len; i++)
> +			used = rte_tel_json_add_array_bool(out_buf,
> +				buf_len, used,
> +				d->data.array[i].boolval);
>   	if (d->type == RTE_TEL_DICT)
>   		for (i = 0; i < d->data_len; i++) {
>   			const struct tel_dict_entry *v = &d->data.dict[i];
> @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>   						buf_len, used,
>   						v->name, v->value.u64val);
>   				break;
> +			case RTE_TEL_BOOL_VAL:
> +				used = rte_tel_json_add_obj_bool(out_buf,
> +						buf_len, used,
> +						v->name, v->value.boolval);
> +				break;
>   			case RTE_TEL_CONTAINER:
>   			{
>   				char temp[buf_len];
> @@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>   						buf_len, used,
>   						v->name, v->value.u64val);
>   				break;
> +			case RTE_TEL_BOOL_VAL:
> +				used = rte_tel_json_add_obj_bool(cb_data_buf,
> +						buf_len, used,
> +						v->name, v->value.boolval);
> +				break;
>   			case RTE_TEL_CONTAINER:
>   			{
>   				char temp[buf_len];
> @@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>   	case RTE_TEL_ARRAY_STRING:
>   	case RTE_TEL_ARRAY_INT:
>   	case RTE_TEL_ARRAY_U64:
> +	case RTE_TEL_ARRAY_BOOL:
>   	case RTE_TEL_ARRAY_CONTAINER:
>   		used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
>   		for (i = 0; i < d->data_len; i++)
> @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>   				used = rte_tel_json_add_array_u64(cb_data_buf,
>   						buf_len, used,
>   						d->data.array[i].u64val);
> +			else if (d->type == RTE_TEL_ARRAY_BOOL)
> +				used = rte_tel_json_add_array_bool(cb_data_buf,
> +						buf_len, used,
> +						d->data.array[i].boolval);
>   			else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
>   				char temp[buf_len];
>   				const struct container *rec_data =
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 34366ecee3..13a7ce7034 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,

Seems like a brittle construct to me. Replace this with a switch statement.

>   	};
>   	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)
> @@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
>   	if (d->type != RTE_TEL_ARRAY_CONTAINER ||
>   			(val->type != RTE_TEL_ARRAY_U64
>   			&& val->type != RTE_TEL_ARRAY_INT
> -			&& val->type != RTE_TEL_ARRAY_STRING))
> +			&& val->type != RTE_TEL_ARRAY_STRING
> +			&& val->type != RTE_TEL_ARRAY_BOOL))

Maybe the introduction of a valid_type() static function would be in 
order? Implemented by means of a (surprise!) switch statement. :)

Could be is_valid_type() and rename valid_name() us_valid_name() in the 
process.

Use in rte_tel_data_add_dict_container() as well.

>   		return -EINVAL;
>   	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
>   		return -ENOSPC;
> @@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
>   	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
>   }
>   
> +int
> +rte_tel_data_add_dict_bool(struct rte_tel_data *d,
> +		const char *name, bool val)
> +{
> +	struct tel_dict_entry *e = &d->data.dict[d->data_len];
> +	if (d->type != RTE_TEL_DICT)
> +		return -EINVAL;
> +	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> +		return -ENOSPC;
> +
> +	if (!valid_name(name))
> +		return -EINVAL;
> +
> +	d->data_len++;
> +	e->type = RTE_TEL_BOOL_VAL;
> +	e->value.boolval = val;
> +	const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> +	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> +}
> +
>   int
>   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
>   		struct rte_tel_data *val, int keep)
> @@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
>   	if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
>   			&& val->type != RTE_TEL_ARRAY_INT
>   			&& val->type != RTE_TEL_ARRAY_STRING
> +			&& val->type != RTE_TEL_ARRAY_BOOL
>   			&& val->type != RTE_TEL_DICT))
>   		return -EINVAL;
>   	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
> index 26aa28e72c..c840486b18 100644
> --- a/lib/telemetry/telemetry_data.h
> +++ b/lib/telemetry/telemetry_data.h
> @@ -5,6 +5,9 @@
>   #ifndef _TELEMETRY_DATA_H_
>   #define _TELEMETRY_DATA_H_
>   
> +#include <stdbool.h>
> +#include <stdint.h>
> +
>   #include "rte_telemetry.h"
>   
>   enum tel_container_types {
> @@ -15,6 +18,7 @@ enum tel_container_types {
>   	RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
>   	RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
>   	RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> +	RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
>   };
>   
>   struct container {
> @@ -30,6 +34,7 @@ union tel_value {
>   	char sval[RTE_TEL_MAX_STRING_LEN];
>   	int ival;
>   	uint64_t u64val;
> +	bool boolval;
>   	struct container container;
>   };
>   
> 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");

Have "true" and "false" as #defines, and make a bool_str() helper, which 
returns the (constant) string form.

> +
> +	ret = __json_snprintf(buf + end, len - end, ",%s]",
> +			val ? "true" : "false");
> +	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;
>   }
>   
> +/**
> + * Add a new element with boolean value to the JSON object stored in the
> + * provided buffer.
> + */
> +static inline int

I know this pattern isn't introduced in this patch, but why is this 
function in the header file?

> +rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
> +		const char *name, bool val)
> +{
> +	int ret, end = used - 1;
> +	if (used <= 2) /* assume empty, since minimum is '{}' */

RTE_ASSERT() wouldn't be more helpful? The input buffer is malformed, 
correct?

Either way, the magic '2' should be a #define.

> +		return __json_snprintf(buf, len, "{\"%s\":%s}", name,
> +				val ? "true" : "false");
> +
> +	ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> +			name, val ? "true" : "false");
> +	return ret == 0 ? used : end + ret;
> +}
> +

This looks like cut-and-paste. Merge into one function, parameterized by 
the start and end delimiter.

>   /**
>    * Add a new element with int value to the JSON object stored in the
>    * provided buffer.
> diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> index 9794f9ea20..88f58d4d89 100644
> --- a/lib/telemetry/version.map
> +++ b/lib/telemetry/version.map
> @@ -19,7 +19,17 @@ DPDK_23 {
>   	local: *;
>   };
>   
> +EXPERIMENTAL {
> +	global:
> +
> +	# added in 22.11
> +	rte_tel_data_add_array_bool;
> +	rte_tel_data_add_dict_bool;
> +};
> +
>   INTERNAL {
> +	global:
> +
>   	rte_telemetry_legacy_register;
>   	rte_telemetry_init;
>   };
  
David Marchand Oct. 25, 2022, 9:43 a.m. UTC | #2
On Tue, Oct 25, 2022 at 11:34 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2022-10-25 11:00, 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>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Ciara Power <ciara.power@intel.com>
> > ---
> > Changes since v1:
> > - fixed doxygen description,
> >
> > ---
> >   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      | 10 ++++
> >   7 files changed, 233 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> > index d92667a527..134e018fde 100644
> > --- a/app/test/test_telemetry_data.c
> > +++ b/app/test/test_telemetry_data.c
> > @@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
> >       return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
> >   }
> >
> > +static int
> > +test_case_array_bool(void)
> > +{
> > +     int i;
> > +
> > +     rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
> > +     for (i = 0; i < 5; i++)
> > +             rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
> > +     return CHECK_OUTPUT("[true,false,true,false,true]");
> > +}
> > +
> > +static int
> > +test_case_add_dict_bool(void)
> > +{
> > +     int i = 0;
> > +     char name_of_value[8];
> > +
> > +     rte_tel_data_start_dict(&response_data);
> > +
> > +     for (i = 0; i < 5; i++) {
> > +             sprintf(name_of_value, "dict_%d", i);
> > +             rte_tel_data_add_dict_bool(&response_data, name_of_value,
> > +                     (i % 2) == 0);
> > +     }
> > +     return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
> > +             "\"dict_3\":false,\"dict_4\":true}");
> > +}
> > +
> > +static int
> > +test_dict_with_array_bool_values(void)
> > +{
> > +     int i;
> > +
> > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > +
> > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > +
> > +     rte_tel_data_start_dict(&response_data);
> > +
> > +     for (i = 0; i < 10; i++) {
> > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > +     }
> > +
> > +     rte_tel_data_add_dict_container(&response_data, "dict_0",
> > +      child_data, 0);
> > +     rte_tel_data_add_dict_container(&response_data, "dict_1",
> > +      child_data2, 0);
> > +
> > +     return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
> > +             "\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
> > +}
> > +
> > +static int
> > +test_array_with_array_bool_values(void)
> > +{
> > +     int i;
> > +
> > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > +
> > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > +
> > +     rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> > +
> > +     for (i = 0; i < 5; i++) {
> > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > +     }
> > +     rte_tel_data_add_array_container(&response_data, child_data, 0);
> > +     rte_tel_data_add_array_container(&response_data, child_data2, 0);
> > +
> > +     return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
> > +}
> > +
> >   static int
> >   test_string_char_escaping(void)
> >   {
> > @@ -428,15 +506,21 @@ telemetry_data_autotest(void)
> >                       test_null_return,
> >                       test_simple_string,
> >                       test_case_array_string,
> > -                     test_case_array_int, test_case_array_u64,
> > -                     test_case_add_dict_int, test_case_add_dict_u64,
> > +                     test_case_array_int,
> > +                     test_case_array_u64,
> > +                     test_case_array_bool,
> > +                     test_case_add_dict_int,
> > +                     test_case_add_dict_u64,
> > +                     test_case_add_dict_bool,
> >                       test_case_add_dict_string,
> >                       test_dict_with_array_int_values,
> >                       test_dict_with_array_u64_values,
> > +                     test_dict_with_array_bool_values,
> >                       test_dict_with_array_string_values,
> >                       test_dict_with_dict_values,
> >                       test_array_with_array_int_values,
> >                       test_array_with_array_u64_values,
> > +                     test_array_with_array_bool_values,
> >                       test_array_with_array_string_values,
> >                       test_string_char_escaping,
> >                       test_array_char_escaping,
> > diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> > index a0d21d6b7f..e7f6c2ae43 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 boolean value to be returned in the array
> > + * @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().
>
> ...as RTE_TEL_BOOL_VAL as the type parameter?
>
> > + *
> > + * @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 boolean value to be stored in the dict
> > + * @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
> > index 8fbb4f3060..276d0f337d 100644
> > --- a/lib/telemetry/telemetry.c
> > +++ b/lib/telemetry/telemetry.c
> > @@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> >       unsigned int i;
> >
> >       if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
> > -             d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
> > +                     d->type != RTE_TEL_ARRAY_INT &&
> > +                     d->type != RTE_TEL_ARRAY_STRING &&
> > +                     d->type != RTE_TEL_ARRAY_BOOL)
>
> Use a switch for improved readability.
>
> >               return snprintf(out_buf, buf_len, "null");
> >
> >       used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> > @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> >                       used = rte_tel_json_add_array_string(out_buf,
> >                               buf_len, used,
> >                               d->data.array[i].sval);
> > +     if (d->type == RTE_TEL_ARRAY_BOOL)
>
> This whole section should be refactor to use a switch statement, imo.
>
> > +             for (i = 0; i < d->data_len; i++)
> > +                     used = rte_tel_json_add_array_bool(out_buf,
> > +                             buf_len, used,
> > +                             d->data.array[i].boolval);
> >       if (d->type == RTE_TEL_DICT)
> >               for (i = 0; i < d->data_len; i++) {
> >                       const struct tel_dict_entry *v = &d->data.dict[i];
> > @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> >                                               buf_len, used,
> >                                               v->name, v->value.u64val);
> >                               break;
> > +                     case RTE_TEL_BOOL_VAL:
> > +                             used = rte_tel_json_add_obj_bool(out_buf,
> > +                                             buf_len, used,
> > +                                             v->name, v->value.boolval);
> > +                             break;
> >                       case RTE_TEL_CONTAINER:
> >                       {
> >                               char temp[buf_len];
> > @@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >                                               buf_len, used,
> >                                               v->name, v->value.u64val);
> >                               break;
> > +                     case RTE_TEL_BOOL_VAL:
> > +                             used = rte_tel_json_add_obj_bool(cb_data_buf,
> > +                                             buf_len, used,
> > +                                             v->name, v->value.boolval);
> > +                             break;
> >                       case RTE_TEL_CONTAINER:
> >                       {
> >                               char temp[buf_len];
> > @@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >       case RTE_TEL_ARRAY_STRING:
> >       case RTE_TEL_ARRAY_INT:
> >       case RTE_TEL_ARRAY_U64:
> > +     case RTE_TEL_ARRAY_BOOL:
> >       case RTE_TEL_ARRAY_CONTAINER:
> >               used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
> >               for (i = 0; i < d->data_len; i++)
> > @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >                               used = rte_tel_json_add_array_u64(cb_data_buf,
> >                                               buf_len, used,
> >                                               d->data.array[i].u64val);
> > +                     else if (d->type == RTE_TEL_ARRAY_BOOL)
> > +                             used = rte_tel_json_add_array_bool(cb_data_buf,
> > +                                             buf_len, used,
> > +                                             d->data.array[i].boolval);
> >                       else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
> >                               char temp[buf_len];
> >                               const struct container *rec_data =
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 34366ecee3..13a7ce7034 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,
>
> Seems like a brittle construct to me. Replace this with a switch statement.
>
> >       };
> >       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)
> > @@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
> >       if (d->type != RTE_TEL_ARRAY_CONTAINER ||
> >                       (val->type != RTE_TEL_ARRAY_U64
> >                       && val->type != RTE_TEL_ARRAY_INT
> > -                     && val->type != RTE_TEL_ARRAY_STRING))
> > +                     && val->type != RTE_TEL_ARRAY_STRING
> > +                     && val->type != RTE_TEL_ARRAY_BOOL))
>
> Maybe the introduction of a valid_type() static function would be in
> order? Implemented by means of a (surprise!) switch statement. :)
>
> Could be is_valid_type() and rename valid_name() us_valid_name() in the
> process.
>
> Use in rte_tel_data_add_dict_container() as well.
>
> >               return -EINVAL;
> >       if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> >               return -ENOSPC;
> > @@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
> >       return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> >   }
> >
> > +int
> > +rte_tel_data_add_dict_bool(struct rte_tel_data *d,
> > +             const char *name, bool val)
> > +{
> > +     struct tel_dict_entry *e = &d->data.dict[d->data_len];
> > +     if (d->type != RTE_TEL_DICT)
> > +             return -EINVAL;
> > +     if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > +             return -ENOSPC;
> > +
> > +     if (!valid_name(name))
> > +             return -EINVAL;
> > +
> > +     d->data_len++;
> > +     e->type = RTE_TEL_BOOL_VAL;
> > +     e->value.boolval = val;
> > +     const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> > +     return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> > +}
> > +
> >   int
> >   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> >               struct rte_tel_data *val, int keep)
> > @@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> >       if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
> >                       && val->type != RTE_TEL_ARRAY_INT
> >                       && val->type != RTE_TEL_ARRAY_STRING
> > +                     && val->type != RTE_TEL_ARRAY_BOOL
> >                       && val->type != RTE_TEL_DICT))
> >               return -EINVAL;
> >       if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
> > index 26aa28e72c..c840486b18 100644
> > --- a/lib/telemetry/telemetry_data.h
> > +++ b/lib/telemetry/telemetry_data.h
> > @@ -5,6 +5,9 @@
> >   #ifndef _TELEMETRY_DATA_H_
> >   #define _TELEMETRY_DATA_H_
> >
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> >   #include "rte_telemetry.h"
> >
> >   enum tel_container_types {
> > @@ -15,6 +18,7 @@ enum tel_container_types {
> >       RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
> >       RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
> >       RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> > +     RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
> >   };
> >
> >   struct container {
> > @@ -30,6 +34,7 @@ union tel_value {
> >       char sval[RTE_TEL_MAX_STRING_LEN];
> >       int ival;
> >       uint64_t u64val;
> > +     bool boolval;
> >       struct container container;
> >   };
> >
> > 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");
>
> Have "true" and "false" as #defines, and make a bool_str() helper, which
> returns the (constant) string form.
>
> > +
> > +     ret = __json_snprintf(buf + end, len - end, ",%s]",
> > +                     val ? "true" : "false");
> > +     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;
> >   }
> >
> > +/**
> > + * Add a new element with boolean value to the JSON object stored in the
> > + * provided buffer.
> > + */
> > +static inline int
>
> I know this pattern isn't introduced in this patch, but why is this
> function in the header file?
>
> > +rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
> > +             const char *name, bool val)
> > +{
> > +     int ret, end = used - 1;
> > +     if (used <= 2) /* assume empty, since minimum is '{}' */
>
> RTE_ASSERT() wouldn't be more helpful? The input buffer is malformed,
> correct?
>
> Either way, the magic '2' should be a #define.
>
> > +             return __json_snprintf(buf, len, "{\"%s\":%s}", name,
> > +                             val ? "true" : "false");
> > +
> > +     ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> > +                     name, val ? "true" : "false");
> > +     return ret == 0 ? used : end + ret;
> > +}
> > +
>
> This looks like cut-and-paste. Merge into one function, parameterized by
> the start and end delimiter.
>
> >   /**
> >    * Add a new element with int value to the JSON object stored in the
> >    * provided buffer.
> > diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> > index 9794f9ea20..88f58d4d89 100644
> > --- a/lib/telemetry/version.map
> > +++ b/lib/telemetry/version.map
> > @@ -19,7 +19,17 @@ DPDK_23 {
> >       local: *;
> >   };
> >
> > +EXPERIMENTAL {
> > +     global:
> > +
> > +     # added in 22.11
> > +     rte_tel_data_add_array_bool;
> > +     rte_tel_data_add_dict_bool;
> > +};
> > +
> >   INTERNAL {
> > +     global:
> > +
> >       rte_telemetry_legacy_register;
> >       rte_telemetry_init;
> >   };
>

I agree with most of the comments.
I can follow up on those cleanups, but I don't have the time now.

I'll let the telemetry maintainers decide on the fate of those patches.
Thanks.
  
Bruce Richardson Oct. 25, 2022, 10:38 a.m. UTC | #3
On Tue, Oct 25, 2022 at 11:43:27AM +0200, David Marchand wrote:
> On Tue, Oct 25, 2022 at 11:34 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> >
> > On 2022-10-25 11:00, 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>
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Acked-by: Ciara Power <ciara.power@intel.com>
> > > ---
> > > Changes since v1:
> > > - fixed doxygen description,
> > >
> > > ---
> > >   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      | 10 ++++
> > >   7 files changed, 233 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> > > index d92667a527..134e018fde 100644
> > > --- a/app/test/test_telemetry_data.c
> > > +++ b/app/test/test_telemetry_data.c
> > > @@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
> > >       return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
> > >   }
> > >
> > > +static int
> > > +test_case_array_bool(void)
> > > +{
> > > +     int i;
> > > +
> > > +     rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
> > > +     for (i = 0; i < 5; i++)
> > > +             rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
> > > +     return CHECK_OUTPUT("[true,false,true,false,true]");
> > > +}
> > > +
> > > +static int
> > > +test_case_add_dict_bool(void)
> > > +{
> > > +     int i = 0;
> > > +     char name_of_value[8];
> > > +
> > > +     rte_tel_data_start_dict(&response_data);
> > > +
> > > +     for (i = 0; i < 5; i++) {
> > > +             sprintf(name_of_value, "dict_%d", i);
> > > +             rte_tel_data_add_dict_bool(&response_data, name_of_value,
> > > +                     (i % 2) == 0);
> > > +     }
> > > +     return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
> > > +             "\"dict_3\":false,\"dict_4\":true}");
> > > +}
> > > +
> > > +static int
> > > +test_dict_with_array_bool_values(void)
> > > +{
> > > +     int i;
> > > +
> > > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > > +
> > > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > > +
> > > +     rte_tel_data_start_dict(&response_data);
> > > +
> > > +     for (i = 0; i < 10; i++) {
> > > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > > +     }
> > > +
> > > +     rte_tel_data_add_dict_container(&response_data, "dict_0",
> > > +      child_data, 0);
> > > +     rte_tel_data_add_dict_container(&response_data, "dict_1",
> > > +      child_data2, 0);
> > > +
> > > +     return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
> > > +             "\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
> > > +}
> > > +
> > > +static int
> > > +test_array_with_array_bool_values(void)
> > > +{
> > > +     int i;
> > > +
> > > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > > +
> > > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > > +
> > > +     rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> > > +
> > > +     for (i = 0; i < 5; i++) {
> > > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > > +     }
> > > +     rte_tel_data_add_array_container(&response_data, child_data, 0);
> > > +     rte_tel_data_add_array_container(&response_data, child_data2, 0);
> > > +
> > > +     return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
> > > +}
> > > +
> > >   static int
> > >   test_string_char_escaping(void)
> > >   {
> > > @@ -428,15 +506,21 @@ telemetry_data_autotest(void)
> > >                       test_null_return,
> > >                       test_simple_string,
> > >                       test_case_array_string,
> > > -                     test_case_array_int, test_case_array_u64,
> > > -                     test_case_add_dict_int, test_case_add_dict_u64,
> > > +                     test_case_array_int,
> > > +                     test_case_array_u64,
> > > +                     test_case_array_bool,
> > > +                     test_case_add_dict_int,
> > > +                     test_case_add_dict_u64,
> > > +                     test_case_add_dict_bool,
> > >                       test_case_add_dict_string,
> > >                       test_dict_with_array_int_values,
> > >                       test_dict_with_array_u64_values,
> > > +                     test_dict_with_array_bool_values,
> > >                       test_dict_with_array_string_values,
> > >                       test_dict_with_dict_values,
> > >                       test_array_with_array_int_values,
> > >                       test_array_with_array_u64_values,
> > > +                     test_array_with_array_bool_values,
> > >                       test_array_with_array_string_values,
> > >                       test_string_char_escaping,
> > >                       test_array_char_escaping,
> > > diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> > > index a0d21d6b7f..e7f6c2ae43 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 boolean value to be returned in the array
> > > + * @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().
> >
> > ...as RTE_TEL_BOOL_VAL as the type parameter?
> >
> > > + *
> > > + * @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 boolean value to be stored in the dict
> > > + * @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
> > > index 8fbb4f3060..276d0f337d 100644
> > > --- a/lib/telemetry/telemetry.c
> > > +++ b/lib/telemetry/telemetry.c
> > > @@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> > >       unsigned int i;
> > >
> > >       if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
> > > -             d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
> > > +                     d->type != RTE_TEL_ARRAY_INT &&
> > > +                     d->type != RTE_TEL_ARRAY_STRING &&
> > > +                     d->type != RTE_TEL_ARRAY_BOOL)
> >
> > Use a switch for improved readability.
> >
> > >               return snprintf(out_buf, buf_len, "null");
> > >
> > >       used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> > > @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> > >                       used = rte_tel_json_add_array_string(out_buf,
> > >                               buf_len, used,
> > >                               d->data.array[i].sval);
> > > +     if (d->type == RTE_TEL_ARRAY_BOOL)
> >
> > This whole section should be refactor to use a switch statement, imo.
> >
> > > +             for (i = 0; i < d->data_len; i++)
> > > +                     used = rte_tel_json_add_array_bool(out_buf,
> > > +                             buf_len, used,
> > > +                             d->data.array[i].boolval);
> > >       if (d->type == RTE_TEL_DICT)
> > >               for (i = 0; i < d->data_len; i++) {
> > >                       const struct tel_dict_entry *v = &d->data.dict[i];
> > > @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> > >                                               buf_len, used,
> > >                                               v->name, v->value.u64val);
> > >                               break;
> > > +                     case RTE_TEL_BOOL_VAL:
> > > +                             used = rte_tel_json_add_obj_bool(out_buf,
> > > +                                             buf_len, used,
> > > +                                             v->name, v->value.boolval);
> > > +                             break;
> > >                       case RTE_TEL_CONTAINER:
> > >                       {
> > >                               char temp[buf_len];
> > > @@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> > >                                               buf_len, used,
> > >                                               v->name, v->value.u64val);
> > >                               break;
> > > +                     case RTE_TEL_BOOL_VAL:
> > > +                             used = rte_tel_json_add_obj_bool(cb_data_buf,
> > > +                                             buf_len, used,
> > > +                                             v->name, v->value.boolval);
> > > +                             break;
> > >                       case RTE_TEL_CONTAINER:
> > >                       {
> > >                               char temp[buf_len];
> > > @@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> > >       case RTE_TEL_ARRAY_STRING:
> > >       case RTE_TEL_ARRAY_INT:
> > >       case RTE_TEL_ARRAY_U64:
> > > +     case RTE_TEL_ARRAY_BOOL:
> > >       case RTE_TEL_ARRAY_CONTAINER:
> > >               used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
> > >               for (i = 0; i < d->data_len; i++)
> > > @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> > >                               used = rte_tel_json_add_array_u64(cb_data_buf,
> > >                                               buf_len, used,
> > >                                               d->data.array[i].u64val);
> > > +                     else if (d->type == RTE_TEL_ARRAY_BOOL)
> > > +                             used = rte_tel_json_add_array_bool(cb_data_buf,
> > > +                                             buf_len, used,
> > > +                                             d->data.array[i].boolval);
> > >                       else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
> > >                               char temp[buf_len];
> > >                               const struct container *rec_data =
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index 34366ecee3..13a7ce7034 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,
> >
> > Seems like a brittle construct to me. Replace this with a switch statement.
> >
> > >       };
> > >       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)
> > > @@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
> > >       if (d->type != RTE_TEL_ARRAY_CONTAINER ||
> > >                       (val->type != RTE_TEL_ARRAY_U64
> > >                       && val->type != RTE_TEL_ARRAY_INT
> > > -                     && val->type != RTE_TEL_ARRAY_STRING))
> > > +                     && val->type != RTE_TEL_ARRAY_STRING
> > > +                     && val->type != RTE_TEL_ARRAY_BOOL))
> >
> > Maybe the introduction of a valid_type() static function would be in
> > order? Implemented by means of a (surprise!) switch statement. :)
> >
> > Could be is_valid_type() and rename valid_name() us_valid_name() in the
> > process.
> >
> > Use in rte_tel_data_add_dict_container() as well.
> >
> > >               return -EINVAL;
> > >       if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> > >               return -ENOSPC;
> > > @@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
> > >       return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> > >   }
> > >
> > > +int
> > > +rte_tel_data_add_dict_bool(struct rte_tel_data *d,
> > > +             const char *name, bool val)
> > > +{
> > > +     struct tel_dict_entry *e = &d->data.dict[d->data_len];
> > > +     if (d->type != RTE_TEL_DICT)
> > > +             return -EINVAL;
> > > +     if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > > +             return -ENOSPC;
> > > +
> > > +     if (!valid_name(name))
> > > +             return -EINVAL;
> > > +
> > > +     d->data_len++;
> > > +     e->type = RTE_TEL_BOOL_VAL;
> > > +     e->value.boolval = val;
> > > +     const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> > > +     return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> > > +}
> > > +
> > >   int
> > >   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> > >               struct rte_tel_data *val, int keep)
> > > @@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> > >       if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
> > >                       && val->type != RTE_TEL_ARRAY_INT
> > >                       && val->type != RTE_TEL_ARRAY_STRING
> > > +                     && val->type != RTE_TEL_ARRAY_BOOL
> > >                       && val->type != RTE_TEL_DICT))
> > >               return -EINVAL;
> > >       if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > > diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
> > > index 26aa28e72c..c840486b18 100644
> > > --- a/lib/telemetry/telemetry_data.h
> > > +++ b/lib/telemetry/telemetry_data.h
> > > @@ -5,6 +5,9 @@
> > >   #ifndef _TELEMETRY_DATA_H_
> > >   #define _TELEMETRY_DATA_H_
> > >
> > > +#include <stdbool.h>
> > > +#include <stdint.h>
> > > +
> > >   #include "rte_telemetry.h"
> > >
> > >   enum tel_container_types {
> > > @@ -15,6 +18,7 @@ enum tel_container_types {
> > >       RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
> > >       RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
> > >       RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> > > +     RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
> > >   };
> > >
> > >   struct container {
> > > @@ -30,6 +34,7 @@ union tel_value {
> > >       char sval[RTE_TEL_MAX_STRING_LEN];
> > >       int ival;
> > >       uint64_t u64val;
> > > +     bool boolval;
> > >       struct container container;
> > >   };
> > >
> > > 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");
> >
> > Have "true" and "false" as #defines, and make a bool_str() helper, which
> > returns the (constant) string form.
> >
> > > +
> > > +     ret = __json_snprintf(buf + end, len - end, ",%s]",
> > > +                     val ? "true" : "false");
> > > +     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;
> > >   }
> > >
> > > +/**
> > > + * Add a new element with boolean value to the JSON object stored in the
> > > + * provided buffer.
> > > + */
> > > +static inline int
> >
> > I know this pattern isn't introduced in this patch, but why is this
> > function in the header file?
> >
> > > +rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
> > > +             const char *name, bool val)
> > > +{
> > > +     int ret, end = used - 1;
> > > +     if (used <= 2) /* assume empty, since minimum is '{}' */
> >
> > RTE_ASSERT() wouldn't be more helpful? The input buffer is malformed,
> > correct?
> >
> > Either way, the magic '2' should be a #define.
> >
> > > +             return __json_snprintf(buf, len, "{\"%s\":%s}", name,
> > > +                             val ? "true" : "false");
> > > +
> > > +     ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> > > +                     name, val ? "true" : "false");
> > > +     return ret == 0 ? used : end + ret;
> > > +}
> > > +
> >
> > This looks like cut-and-paste. Merge into one function, parameterized by
> > the start and end delimiter.
> >
> > >   /**
> > >    * Add a new element with int value to the JSON object stored in the
> > >    * provided buffer.
> > > diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> > > index 9794f9ea20..88f58d4d89 100644
> > > --- a/lib/telemetry/version.map
> > > +++ b/lib/telemetry/version.map
> > > @@ -19,7 +19,17 @@ DPDK_23 {
> > >       local: *;
> > >   };
> > >
> > > +EXPERIMENTAL {
> > > +     global:
> > > +
> > > +     # added in 22.11
> > > +     rte_tel_data_add_array_bool;
> > > +     rte_tel_data_add_dict_bool;
> > > +};
> > > +
> > >   INTERNAL {
> > > +     global:
> > > +
> > >       rte_telemetry_legacy_register;
> > >       rte_telemetry_init;
> > >   };
> >
> 
> I agree with most of the comments.
> I can follow up on those cleanups, but I don't have the time now.
> 
> I'll let the telemetry maintainers decide on the fate of those patches.
> Thanks.
> 
Well, given the lateness in the release process, if it's a choice between
getting the patches in as-is, vs not getting them in at all, I think having
them in 22.11 is the better choice. While there is cleanup that can be
done, I don't see this patch as making the overall code any worse.

My 2c.

/Bruce
  
Ciara Power Oct. 27, 2022, 8:53 a.m. UTC | #4
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Tuesday 25 October 2022 11:39
> To: David Marchand <david.marchand@redhat.com>
> Cc: Mattias Rönnblom <hofors@lysator.liu.se>; dev@dpdk.org; Morten
> Brørup <mb@smartsharesystems.com>; Power, Ciara
> <ciara.power@intel.com>
> Subject: Re: [PATCH v3 1/4] telemetry: support boolean type
> 
> On Tue, Oct 25, 2022 at 11:43:27AM +0200, David Marchand wrote:
> > On Tue, Oct 25, 2022 at 11:34 AM Mattias Rönnblom
> <hofors@lysator.liu.se> wrote:
> > >
> > > On 2022-10-25 11:00, 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>
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > Acked-by: Ciara Power <ciara.power@intel.com>
> > > > ---
> > > > Changes since v1:
> > > > - fixed doxygen description,
<snip>
> > I'll let the telemetry maintainers decide on the fate of those patches.
> > Thanks.
> >
> Well, given the lateness in the release process, if it's a choice between
> getting the patches in as-is, vs not getting them in at all, I think having them
> in 22.11 is the better choice. While there is cleanup that can be done, I don't
> see this patch as making the overall code any worse.
> 
> My 2c.
> 
> /Bruce

Agreed, it's a good addition to the library, +1 to having it in 22.11

Thanks,
Ciara
  

Patch

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index d92667a527..134e018fde 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -353,6 +353,84 @@  test_array_with_array_u64_values(void)
 	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
+static int
+test_case_array_bool(void)
+{
+	int i;
+
+	rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
+	for (i = 0; i < 5; i++)
+		rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
+	return CHECK_OUTPUT("[true,false,true,false,true]");
+}
+
+static int
+test_case_add_dict_bool(void)
+{
+	int i = 0;
+	char name_of_value[8];
+
+	rte_tel_data_start_dict(&response_data);
+
+	for (i = 0; i < 5; i++) {
+		sprintf(name_of_value, "dict_%d", i);
+		rte_tel_data_add_dict_bool(&response_data, name_of_value,
+			(i % 2) == 0);
+	}
+	return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
+		"\"dict_3\":false,\"dict_4\":true}");
+}
+
+static int
+test_dict_with_array_bool_values(void)
+{
+	int i;
+
+	struct rte_tel_data *child_data = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
+
+	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
+
+	rte_tel_data_start_dict(&response_data);
+
+	for (i = 0; i < 10; i++) {
+		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
+		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
+	}
+
+	rte_tel_data_add_dict_container(&response_data, "dict_0",
+	 child_data, 0);
+	rte_tel_data_add_dict_container(&response_data, "dict_1",
+	 child_data2, 0);
+
+	return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
+		"\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
+}
+
+static int
+test_array_with_array_bool_values(void)
+{
+	int i;
+
+	struct rte_tel_data *child_data = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
+
+	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
+
+	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
+
+	for (i = 0; i < 5; i++) {
+		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
+		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
+	}
+	rte_tel_data_add_array_container(&response_data, child_data, 0);
+	rte_tel_data_add_array_container(&response_data, child_data2, 0);
+
+	return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
+}
+
 static int
 test_string_char_escaping(void)
 {
@@ -428,15 +506,21 @@  telemetry_data_autotest(void)
 			test_null_return,
 			test_simple_string,
 			test_case_array_string,
-			test_case_array_int, test_case_array_u64,
-			test_case_add_dict_int, test_case_add_dict_u64,
+			test_case_array_int,
+			test_case_array_u64,
+			test_case_array_bool,
+			test_case_add_dict_int,
+			test_case_add_dict_u64,
+			test_case_add_dict_bool,
 			test_case_add_dict_string,
 			test_dict_with_array_int_values,
 			test_dict_with_array_u64_values,
+			test_dict_with_array_bool_values,
 			test_dict_with_array_string_values,
 			test_dict_with_dict_values,
 			test_array_with_array_int_values,
 			test_array_with_array_u64_values,
+			test_array_with_array_bool_values,
 			test_array_with_array_string_values,
 			test_string_char_escaping,
 			test_array_char_escaping,
diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index a0d21d6b7f..e7f6c2ae43 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 boolean value to be returned in the array
+ * @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 boolean value to be stored in the dict
+ * @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
index 8fbb4f3060..276d0f337d 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -168,7 +168,9 @@  container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 	unsigned int i;
 
 	if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
-		d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
+			d->type != RTE_TEL_ARRAY_INT &&
+			d->type != RTE_TEL_ARRAY_STRING &&
+			d->type != RTE_TEL_ARRAY_BOOL)
 		return snprintf(out_buf, buf_len, "null");
 
 	used = rte_tel_json_empty_array(out_buf, buf_len, 0);
@@ -187,6 +189,11 @@  container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 			used = rte_tel_json_add_array_string(out_buf,
 				buf_len, used,
 				d->data.array[i].sval);
+	if (d->type == RTE_TEL_ARRAY_BOOL)
+		for (i = 0; i < d->data_len; i++)
+			used = rte_tel_json_add_array_bool(out_buf,
+				buf_len, used,
+				d->data.array[i].boolval);
 	if (d->type == RTE_TEL_DICT)
 		for (i = 0; i < d->data_len; i++) {
 			const struct tel_dict_entry *v = &d->data.dict[i];
@@ -206,6 +213,11 @@  container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 						buf_len, used,
 						v->name, v->value.u64val);
 				break;
+			case RTE_TEL_BOOL_VAL:
+				used = rte_tel_json_add_obj_bool(out_buf,
+						buf_len, used,
+						v->name, v->value.boolval);
+				break;
 			case RTE_TEL_CONTAINER:
 			{
 				char temp[buf_len];
@@ -273,6 +285,11 @@  output_json(const char *cmd, const struct rte_tel_data *d, int s)
 						buf_len, used,
 						v->name, v->value.u64val);
 				break;
+			case RTE_TEL_BOOL_VAL:
+				used = rte_tel_json_add_obj_bool(cb_data_buf,
+						buf_len, used,
+						v->name, v->value.boolval);
+				break;
 			case RTE_TEL_CONTAINER:
 			{
 				char temp[buf_len];
@@ -294,6 +311,7 @@  output_json(const char *cmd, const struct rte_tel_data *d, int s)
 	case RTE_TEL_ARRAY_STRING:
 	case RTE_TEL_ARRAY_INT:
 	case RTE_TEL_ARRAY_U64:
+	case RTE_TEL_ARRAY_BOOL:
 	case RTE_TEL_ARRAY_CONTAINER:
 		used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
 		for (i = 0; i < d->data_len; i++)
@@ -310,6 +328,10 @@  output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				used = rte_tel_json_add_array_u64(cb_data_buf,
 						buf_len, used,
 						d->data.array[i].u64val);
+			else if (d->type == RTE_TEL_ARRAY_BOOL)
+				used = rte_tel_json_add_array_bool(cb_data_buf,
+						buf_len, used,
+						d->data.array[i].boolval);
 			else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
 				char temp[buf_len];
 				const struct container *rec_data =
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 34366ecee3..13a7ce7034 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,
 	};
 	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)
@@ -87,7 +99,8 @@  rte_tel_data_add_array_container(struct rte_tel_data *d,
 	if (d->type != RTE_TEL_ARRAY_CONTAINER ||
 			(val->type != RTE_TEL_ARRAY_U64
 			&& val->type != RTE_TEL_ARRAY_INT
-			&& val->type != RTE_TEL_ARRAY_STRING))
+			&& val->type != RTE_TEL_ARRAY_STRING
+			&& val->type != RTE_TEL_ARRAY_BOOL))
 		return -EINVAL;
 	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
 		return -ENOSPC;
@@ -179,6 +192,26 @@  rte_tel_data_add_dict_u64(struct rte_tel_data *d,
 	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
 }
 
+int
+rte_tel_data_add_dict_bool(struct rte_tel_data *d,
+		const char *name, bool val)
+{
+	struct tel_dict_entry *e = &d->data.dict[d->data_len];
+	if (d->type != RTE_TEL_DICT)
+		return -EINVAL;
+	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
+		return -ENOSPC;
+
+	if (!valid_name(name))
+		return -EINVAL;
+
+	d->data_len++;
+	e->type = RTE_TEL_BOOL_VAL;
+	e->value.boolval = val;
+	const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
+	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
+}
+
 int
 rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 		struct rte_tel_data *val, int keep)
@@ -188,6 +221,7 @@  rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 	if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
 			&& val->type != RTE_TEL_ARRAY_INT
 			&& val->type != RTE_TEL_ARRAY_STRING
+			&& val->type != RTE_TEL_ARRAY_BOOL
 			&& val->type != RTE_TEL_DICT))
 		return -EINVAL;
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
index 26aa28e72c..c840486b18 100644
--- a/lib/telemetry/telemetry_data.h
+++ b/lib/telemetry/telemetry_data.h
@@ -5,6 +5,9 @@ 
 #ifndef _TELEMETRY_DATA_H_
 #define _TELEMETRY_DATA_H_
 
+#include <stdbool.h>
+#include <stdint.h>
+
 #include "rte_telemetry.h"
 
 enum tel_container_types {
@@ -15,6 +18,7 @@  enum tel_container_types {
 	RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
 	RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
 	RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
+	RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
 };
 
 struct container {
@@ -30,6 +34,7 @@  union tel_value {
 	char sval[RTE_TEL_MAX_STRING_LEN];
 	int ival;
 	uint64_t u64val;
+	bool boolval;
 	struct container container;
 };
 
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");
+	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;
 }
 
+/**
+ * Add a new element with boolean value to the JSON object stored in the
+ * provided buffer.
+ */
+static inline int
+rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
+		const char *name, bool val)
+{
+	int ret, end = used - 1;
+	if (used <= 2) /* assume empty, since minimum is '{}' */
+		return __json_snprintf(buf, len, "{\"%s\":%s}", name,
+				val ? "true" : "false");
+
+	ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
+			name, val ? "true" : "false");
+	return ret == 0 ? used : end + ret;
+}
+
 /**
  * Add a new element with int value to the JSON object stored in the
  * provided buffer.
diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
index 9794f9ea20..88f58d4d89 100644
--- a/lib/telemetry/version.map
+++ b/lib/telemetry/version.map
@@ -19,7 +19,17 @@  DPDK_23 {
 	local: *;
 };
 
+EXPERIMENTAL {
+	global:
+
+	# added in 22.11
+	rte_tel_data_add_array_bool;
+	rte_tel_data_add_dict_bool;
+};
+
 INTERNAL {
+	global:
+
 	rte_telemetry_legacy_register;
 	rte_telemetry_init;
 };