From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5FAA6C04A95 for ; Tue, 25 Oct 2022 09:34:07 +0000 (UTC) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3CBE242BB8; Tue, 25 Oct 2022 11:34:06 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 8465041143 for ; Tue, 25 Oct 2022 11:34:04 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 0F48A13B60 for ; Tue, 25 Oct 2022 11:34:04 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 0E4DC13F0F; Tue, 25 Oct 2022 11:34:04 +0200 (CEST) Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 6EFE913D4B; Tue, 25 Oct 2022 11:34:02 +0200 (CEST) Message-ID: <6de84928-8f39-7e82-44ec-da40eb992380@lysator.liu.se> Date: Tue, 25 Oct 2022 11:34:01 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH v3 1/4] telemetry: support boolean type To: David Marchand , dev@dpdk.org Cc: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Bruce Richardson , Ciara Power References: <20221013074928.3062458-1-david.marchand@redhat.com> <20221025090052.429232-1-david.marchand@redhat.com> <20221025090052.429232-2-david.marchand@redhat.com> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <20221025090052.429232-2-david.marchand@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > Acked-by: Morten Brørup > Acked-by: Bruce Richardson > Acked-by: Ciara Power > --- > 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 > #include > > #include > @@ -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 > +#include > + > #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 > #include > +#include > #include > #include > #include > @@ -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; > };