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.richard...@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.march...@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.richard...@intel.com>
> > Acked-by: Bruce Richardson <bruce.richard...@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
