From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 37B1FA054A; Tue, 25 Oct 2022 11:43:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 25EDB427FF; Tue, 25 Oct 2022 11:43:48 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 1673B41143 for ; Tue, 25 Oct 2022 11:43:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666691026; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UYBDu+m6qyiYprzXuZdGUwFQijZ/gzhpmd0fdG69/rQ=; b=caxmsdQjVW+3E/9bLnZqlsJF+1v82TadsL7fKYbvCRLCb0rkPW2sMnbupDWUb+MUo8IW1t nXbcje2mFF00K/cpTh2/w1lopjgco3s5lhmryUDckwqrEWciRBZy8Wv8kdEOnquj6GmgSW kWV7JnD/ynseRXH14sm1Vt+IW8wKqYE= Received: from mail-pf1-f197.google.com (mail-pf1-f197.google.com [209.85.210.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-534-KGbG9w0zPTKCNhhfYYImHw-1; Tue, 25 Oct 2022 05:43:40 -0400 X-MC-Unique: KGbG9w0zPTKCNhhfYYImHw-1 Received: by mail-pf1-f197.google.com with SMTP id bq6-20020a056a000e0600b0056bcbc6720fso2507383pfb.22 for ; Tue, 25 Oct 2022 02:43:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=UYBDu+m6qyiYprzXuZdGUwFQijZ/gzhpmd0fdG69/rQ=; b=j0TnWSY7QdGPWx29ToMQPItN21Wt6hkWlhC2pcRuui/bLmGIsxNgg+Ssb+RigPos5a H32WZBkuUwKvpI1Kyi5ozOlNb8dSWjuEBsiRmX/a8m8OszEvwFb4cqzrYp3+WIESxKwu FdxRZ+vWAdyzRgMYBzX6FU19sfLDnC9PLR9Jjw1/73n1YGTdOL3vbts4ZTRhSlYdTnvP WdSBAmbQJbrmkggfAEaCUQdQjN8UGtqjkUbbSJOKdCe4hh9tw73BH+1+rxgmStYGPuXb WHuL083weK3P30FsLeylk0CpTClnxejysQRv/3ByHahoBOQvFhft9Gpy249LcZOiapbZ cIpg== X-Gm-Message-State: ACrzQf2u5o06cpyggzAOaZXoVVhwR3Q4Q5lzhi1tW3k6vKuyLcH+xcqr 77jJGMkBq8cLtpZK8OhnBPpBUPXDUOl9n+8Sj17BNXgizNdvmSKHzU+BKNothVg2qaUPW1W/hfb imfEImz8d/Zb+YNIqIyI= X-Received: by 2002:a62:cec8:0:b0:56c:235:8399 with SMTP id y191-20020a62cec8000000b0056c02358399mr5869934pfg.64.1666691018906; Tue, 25 Oct 2022 02:43:38 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6Y+Ybg55n80WhpG8Sg2DttjA9nVuRCqVnPj7jc1Rrsyyr5b1YpXGaZjGWCYHnQpPSpzLIYIxOBZCpSwCUBkEI= X-Received: by 2002:a62:cec8:0:b0:56c:235:8399 with SMTP id y191-20020a62cec8000000b0056c02358399mr5869903pfg.64.1666691018469; Tue, 25 Oct 2022 02:43:38 -0700 (PDT) MIME-Version: 1.0 References: <20221013074928.3062458-1-david.marchand@redhat.com> <20221025090052.429232-1-david.marchand@redhat.com> <20221025090052.429232-2-david.marchand@redhat.com> <6de84928-8f39-7e82-44ec-da40eb992380@lysator.liu.se> In-Reply-To: <6de84928-8f39-7e82-44ec-da40eb992380@lysator.liu.se> From: David Marchand Date: Tue, 25 Oct 2022 11:43:27 +0200 Message-ID: Subject: Re: [PATCH v3 1/4] telemetry: support boolean type To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= Cc: dev@dpdk.org, =?UTF-8?Q?Morten_Br=C3=B8rup?= , Bruce Richardson , Ciara Power X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Tue, Oct 25, 2022 at 11:34 AM Mattias R=C3=B6nnblom 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 > > Acked-by: Morten Br=C3=B8rup > > 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_d= ata.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 =3D 0; i < 5; i++) > > + rte_tel_data_add_array_bool(&response_data, (i % 2) =3D= =3D 0); > > + return CHECK_OUTPUT("[true,false,true,false,true]"); > > +} > > + > > +static int > > +test_case_add_dict_bool(void) > > +{ > > + int i =3D 0; > > + char name_of_value[8]; > > + > > + rte_tel_data_start_dict(&response_data); > > + > > + for (i =3D 0; i < 5; i++) { > > + sprintf(name_of_value, "dict_%d", i); > > + rte_tel_data_add_dict_bool(&response_data, name_of_value, > > + (i % 2) =3D=3D 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 =3D rte_tel_data_alloc(); > > + rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL); > > + > > + struct rte_tel_data *child_data2 =3D rte_tel_data_alloc(); > > + rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL); > > + > > + rte_tel_data_start_dict(&response_data); > > + > > + for (i =3D 0; i < 10; i++) { > > + rte_tel_data_add_array_bool(child_data, (i % 2) =3D=3D 0)= ; > > + rte_tel_data_add_array_bool(child_data2, (i % 2) =3D=3D 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,fals= e,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 =3D rte_tel_data_alloc(); > > + rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL); > > + > > + struct rte_tel_data *child_data2 =3D 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 =3D 0; i < 5; i++) { > > + rte_tel_data_add_array_bool(child_data, (i % 2) =3D=3D 0)= ; > > + rte_tel_data_add_array_bool(child_data2, (i % 2) =3D=3D 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,fa= lse,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_telemetr= y.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 *n= ame, > > 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, b= ool val); > > + > > /** > > * This telemetry callback is used when registering a telemetry comma= nd. > > * It handles getting and formatting information to be returned to te= lemetry > > 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, cha= r *out_buf, size_t buf_len) > > unsigned int i; > > > > if (d->type !=3D RTE_TEL_DICT && d->type !=3D RTE_TEL_ARRAY_U64 &= & > > - d->type !=3D RTE_TEL_ARRAY_INT && d->type !=3D RTE_TEL_AR= RAY_STRING) > > + d->type !=3D RTE_TEL_ARRAY_INT && > > + d->type !=3D RTE_TEL_ARRAY_STRING && > > + d->type !=3D RTE_TEL_ARRAY_BOOL) > > Use a switch for improved readability. > > > return snprintf(out_buf, buf_len, "null"); > > > > used =3D rte_tel_json_empty_array(out_buf, buf_len, 0); > > @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, ch= ar *out_buf, size_t buf_len) > > used =3D rte_tel_json_add_array_string(out_buf, > > buf_len, used, > > d->data.array[i].sval); > > + if (d->type =3D=3D RTE_TEL_ARRAY_BOOL) > > This whole section should be refactor to use a switch statement, imo. > > > + for (i =3D 0; i < d->data_len; i++) > > + used =3D rte_tel_json_add_array_bool(out_buf, > > + buf_len, used, > > + d->data.array[i].boolval); > > if (d->type =3D=3D RTE_TEL_DICT) > > for (i =3D 0; i < d->data_len; i++) { > > const struct tel_dict_entry *v =3D &d->data.dict[= i]; > > @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, ch= ar *out_buf, size_t buf_len) > > buf_len, used, > > v->name, v->value.u64val)= ; > > break; > > + case RTE_TEL_BOOL_VAL: > > + used =3D rte_tel_json_add_obj_bool(out_bu= f, > > + 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 =3D rte_tel_json_add_obj_bool(cb_dat= a_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_d= ata *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 =3D rte_tel_json_empty_array(cb_data_buf, buf_len, 0= ); > > for (i =3D 0; i < d->data_len; i++) > > @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_= data *d, int s) > > used =3D rte_tel_json_add_array_u64(cb_da= ta_buf, > > buf_len, used, > > d->data.array[i].u64val); > > + else if (d->type =3D=3D RTE_TEL_ARRAY_BOOL) > > + used =3D rte_tel_json_add_array_bool(cb_d= ata_buf, > > + buf_len, used, > > + d->data.array[i].boolval)= ; > > else if (d->type =3D=3D RTE_TEL_ARRAY_CONTAINER) = { > > char temp[buf_len]; > > const struct container *rec_data =3D > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_d= ata.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_t= ype type) > > { > > enum tel_container_types array_types[] =3D { > > - RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL =3D 0= */ > > - RTE_TEL_ARRAY_INT, /* RTE_TEL_INT_VAL =3D 1 */ > > - RTE_TEL_ARRAY_U64, /* RTE_TEL_u64_VAL =3D 2 */ > > - RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER =3D= 3 */ > > + [RTE_TEL_STRING_VAL] =3D RTE_TEL_ARRAY_STRING, > > + [RTE_TEL_INT_VAL] =3D RTE_TEL_ARRAY_INT, > > + [RTE_TEL_U64_VAL] =3D RTE_TEL_ARRAY_U64, > > + [RTE_TEL_CONTAINER] =3D RTE_TEL_ARRAY_CONTAINER, > > + [RTE_TEL_BOOL_VAL] =3D RTE_TEL_ARRAY_BOOL, > > Seems like a brittle construct to me. Replace this with a switch statemen= t. > > > }; > > d->type =3D array_types[type]; > > d->data_len =3D 0; > > @@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, u= int64_t x) > > return 0; > > } > > > > +int > > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x) > > +{ > > + if (d->type !=3D RTE_TEL_ARRAY_BOOL) > > + return -EINVAL; > > + if (d->data_len >=3D RTE_TEL_MAX_ARRAY_ENTRIES) > > + return -ENOSPC; > > + d->data.array[d->data_len++].boolval =3D 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 !=3D RTE_TEL_ARRAY_CONTAINER || > > (val->type !=3D RTE_TEL_ARRAY_U64 > > && val->type !=3D RTE_TEL_ARRAY_INT > > - && val->type !=3D RTE_TEL_ARRAY_STRING)) > > + && val->type !=3D RTE_TEL_ARRAY_STRING > > + && val->type !=3D 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 >=3D 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 =3D &d->data.dict[d->data_len]; > > + if (d->type !=3D RTE_TEL_DICT) > > + return -EINVAL; > > + if (d->data_len >=3D RTE_TEL_MAX_DICT_ENTRIES) > > + return -ENOSPC; > > + > > + if (!valid_name(name)) > > + return -EINVAL; > > + > > + d->data_len++; > > + e->type =3D RTE_TEL_BOOL_VAL; > > + e->value.boolval =3D val; > > + const size_t bytes =3D 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 *n= ame, > > 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 !=3D RTE_TEL_DICT || (val->type !=3D RTE_TEL_ARRAY_U6= 4 > > && val->type !=3D RTE_TEL_ARRAY_INT > > && val->type !=3D RTE_TEL_ARRAY_STRING > > + && val->type !=3D RTE_TEL_ARRAY_BOOL > > && val->type !=3D RTE_TEL_DICT)) > > return -EINVAL; > > if (d->data_len >=3D RTE_TEL_MAX_DICT_ENTRIES) > > diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_d= ata.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_j= son.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 le= n, const int used, > > return ret =3D=3D 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 =3D used - 1; /* strip off final delimiter */ > > + if (used <=3D 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 =3D __json_snprintf(buf + end, len - end, ",%s]", > > + val ? "true" : "false"); > > + return ret =3D=3D 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 =3D=3D 0 ? used : end + ret; > > } > > > > +/** > > + * Add a new element with boolean value to the JSON object stored in t= he > > + * 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 =3D used - 1; > > + if (used <=3D 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 =3D __json_snprintf(buf + end, len - end, ",\"%s\":%s}", > > + name, val ? "true" : "false"); > > + return ret =3D=3D 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. --=20 David Marchand