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 <hof...@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.march...@redhat.com>
> > > Acked-by: Morten Brørup <m...@smartsharesystems.com>
> > > Acked-by: Bruce Richardson <bruce.richard...@intel.com>
> > > Acked-by: Ciara Power <ciara.po...@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
