* [PATCH] telemetry: support boolean type
@ 2022-10-19 7:37 David Marchand
2022-10-19 8:13 ` Morten Brørup
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: David Marchand @ 2022-10-19 7:37 UTC (permalink / raw)
To: dev; +Cc: bruce.richardson, mb, Ciara Power
Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
Signed-off-by: David Marchand <david.marchand@redhat.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(-)
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..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
+ * @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
+ * @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 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,
};
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;
@@ -180,6 +193,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)
@@ -189,6 +222,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..b339b1490f 100644
--- a/lib/telemetry/version.map
+++ b/lib/telemetry/version.map
@@ -19,6 +19,11 @@ DPDK_23 {
local: *;
};
+EXPERIMENTAL {
+ rte_tel_data_add_array_bool;
+ rte_tel_data_add_dict_bool;
+};
+
INTERNAL {
rte_telemetry_legacy_register;
rte_telemetry_init;
--
2.37.3
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] telemetry: support boolean type
2022-10-19 7:37 [PATCH] telemetry: support boolean type David Marchand
@ 2022-10-19 8:13 ` Morten Brørup
2022-10-19 13:47 ` Bruce Richardson
2022-10-20 9:54 ` Power, Ciara
2 siblings, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2022-10-19 8:13 UTC (permalink / raw)
To: David Marchand, dev; +Cc: bruce.richardson, Ciara Power
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 19 October 2022 09.37
>
> 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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] telemetry: support boolean type
2022-10-19 7:37 [PATCH] telemetry: support boolean type David Marchand
2022-10-19 8:13 ` Morten Brørup
@ 2022-10-19 13:47 ` Bruce Richardson
2022-10-19 14:18 ` Morten Brørup
2022-10-19 14:28 ` David Marchand
2022-10-20 9:54 ` Power, Ciara
2 siblings, 2 replies; 9+ messages in thread
From: Bruce Richardson @ 2022-10-19 13:47 UTC (permalink / raw)
To: David Marchand; +Cc: dev, mb, Ciara Power
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>
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] telemetry: support boolean type
2022-10-19 13:47 ` Bruce Richardson
@ 2022-10-19 14:18 ` Morten Brørup
2022-10-19 14:28 ` David Marchand
1 sibling, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2022-10-19 14:18 UTC (permalink / raw)
To: Bruce Richardson, David Marchand; +Cc: dev, Ciara Power
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 19 October 2022 15.48
>
> 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>
[...]
> > +/* 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.
A macro will probably degrade source code readability. Please keep as is.
I considered moving the conditional inside the format string to get rid of the %s. But there is no performance requirement, and it is the exception that it can be done for Booleans (but not integers and other types). So I concluded that the current form is good.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] telemetry: support boolean type
2022-10-19 13:47 ` Bruce Richardson
2022-10-19 14:18 ` Morten Brørup
@ 2022-10-19 14:28 ` David Marchand
2022-10-19 16:47 ` Bruce Richardson
1 sibling, 1 reply; 9+ messages in thread
From: David Marchand @ 2022-10-19 14:28 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev, mb, Ciara Power
On Wed, Oct 19, 2022 at 3:48 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> 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
I wondered too, but then I saw that only the "simple" type string was
handled and others were not.
So I decided to skip.
> 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
>
Indeed..
> > + * @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!
I don't remember if this form is related to some CXX standard... but I
see that no compiler complained in the CI.
>
> > 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.
Err, naming it would be hard and I don't see for now how we could reuse it.
>
> > + return ret == 0 ? used : end + ret;
> > +}
> > +
> > /*
> > * Add a new element with raw JSON value to the JSON array stored in the
> > * provided buffer.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] telemetry: support boolean type
2022-10-19 14:28 ` David Marchand
@ 2022-10-19 16:47 ` Bruce Richardson
2022-10-19 18:08 ` Morten Brørup
0 siblings, 1 reply; 9+ messages in thread
From: Bruce Richardson @ 2022-10-19 16:47 UTC (permalink / raw)
To: David Marchand; +Cc: dev, mb, Ciara Power
On Wed, Oct 19, 2022 at 04:28:58PM +0200, David Marchand wrote:
> On Wed, Oct 19, 2022 at 3:48 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > 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
>
> I wondered too, but then I saw that only the "simple" type string was
> handled and others were not.
> So I decided to skip.
>
>
> > 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
> >
>
> Indeed..
>
> > > + * @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!
>
> I don't remember if this form is related to some CXX standard... but I
> see that no compiler complained in the CI.
>
>
> >
> > > 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.
>
> Err, naming it would be hard and I don't see for now how we could reuse it.
>
Yes, and I see Morten has objected from a readability perspective, so
keeping as-is is fine.
One final suggestion though might be to have an array with the strings as so:
const char *bool_str[2] = { "false", "true" };
and then in the code use "bool_str[val]" in place of ternary operator.
(From a quick check with godbolt is looks like bool params are clamped to 0
or 1 on function call, but if we want to be paranoid, we can lookup based
on [!!val])
However, ok to keep code as-is for this too.
/Bruce
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] telemetry: support boolean type
2022-10-19 16:47 ` Bruce Richardson
@ 2022-10-19 18:08 ` Morten Brørup
0 siblings, 0 replies; 9+ messages in thread
From: Morten Brørup @ 2022-10-19 18:08 UTC (permalink / raw)
To: Bruce Richardson, David Marchand; +Cc: dev, Ciara Power
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 19 October 2022 18.48
>
> On Wed, Oct 19, 2022 at 04:28:58PM +0200, David Marchand wrote:
> > On Wed, Oct 19, 2022 at 3:48 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > 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>
> > > > ---
[...]
> > > > +/* 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.
> >
> > Err, naming it would be hard and I don't see for now how we could
> reuse it.
> >
> Yes, and I see Morten has objected from a readability perspective, so
> keeping as-is is fine.
>
> One final suggestion though might be to have an array with the strings
> as so:
>
> const char *bool_str[2] = { "false", "true" };
>
> and then in the code use "bool_str[val]" in place of ternary operator.
I don't consider that more readable than the original.
> (From a quick check with godbolt is looks like bool params are clamped
> to 0
> or 1 on function call, but if we want to be paranoid, we can lookup
> based
> on [!!val])
>
> However, ok to keep code as-is for this too.
Thank you, yes, please.
>
> /Bruce
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] telemetry: support boolean type
2022-10-19 7:37 [PATCH] telemetry: support boolean type David Marchand
2022-10-19 8:13 ` Morten Brørup
2022-10-19 13:47 ` Bruce Richardson
@ 2022-10-20 9:54 ` Power, Ciara
2022-10-20 11:48 ` David Marchand
2 siblings, 1 reply; 9+ messages in thread
From: Power, Ciara @ 2022-10-20 9:54 UTC (permalink / raw)
To: David Marchand, dev; +Cc: Richardson, Bruce, mb
Hi David,
This is a nice addition to the library.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday 19 October 2022 08:37
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>;
> mb@smartsharesystems.com; Power, Ciara <ciara.power@intel.com>
> Subject: [PATCH] telemetry: support boolean type
>
> Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
>
> Signed-off-by: David Marchand <david.marchand@redhat.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
<snip>
Acked-by: Ciara Power <ciara.power@intel.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] telemetry: support boolean type
2022-10-20 9:54 ` Power, Ciara
@ 2022-10-20 11:48 ` David Marchand
0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2022-10-20 11:48 UTC (permalink / raw)
To: Power, Ciara; +Cc: dev, Richardson, Bruce, mb
Hello Ciara,
On Thu, Oct 20, 2022 at 11:54 AM Power, Ciara <ciara.power@intel.com> wrote:
> This is a nice addition to the library.
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Wednesday 19 October 2022 08:37
> > To: dev@dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>;
> > mb@smartsharesystems.com; Power, Ciara <ciara.power@intel.com>
> > Subject: [PATCH] telemetry: support boolean type
> >
> > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.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
> <snip>
>
> Acked-by: Ciara Power <ciara.power@intel.com>
Thank you.
Fyi, I'll respin this patch to fix Bruce comments.
I will send it as preparation for the trace telemetry additions.
--
David Marchand
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-20 11:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-19 7:37 [PATCH] telemetry: support boolean type David Marchand
2022-10-19 8:13 ` Morten Brørup
2022-10-19 13:47 ` Bruce Richardson
2022-10-19 14:18 ` Morten Brørup
2022-10-19 14:28 ` David Marchand
2022-10-19 16:47 ` Bruce Richardson
2022-10-19 18:08 ` Morten Brørup
2022-10-20 9:54 ` Power, Ciara
2022-10-20 11:48 ` David Marchand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).