From: Bruce Richardson <bruce.richardson@intel.com>
To: Huisong Li <lihuisong@huawei.com>
Cc: <dev@dpdk.org>, <mb@smartsharesystems.com>,
<andrew.rybchenko@oktetlabs.ru>, <huangdaode@huawei.com>,
<liudongdong3@huawei.com>, <fengchengwen@huawei.com>
Subject: Re: [PATCH V4 7/9] telemetry: support adding integer value as hexadecimal
Date: Tue, 13 Dec 2022 17:09:10 +0000 [thread overview]
Message-ID: <Y5ixtryyO/0u2nym@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20221213101512.39919-8-lihuisong@huawei.com>
On Tue, Dec 13, 2022 at 06:15:10PM +0800, Huisong Li wrote:
> Sometimes displaying a unsigned integer value as hexadecimal encoded style
> is more expected for human consumption, such as, offload capability and
> device flag. This patch introduces two APIs to add unsigned integer (can be
> one of uint8_t, uint16_t, uint32_t and uint64_t type) value as hexadecimal
> encoded string to array or dictionary. If the 'val_bits' is zero, the value
> is stored as hexadecimal encoded string without padding zero.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
Thanks for the patch. Agree with the principle of it, but some comments
inline.
/Bruce
> ---
> lib/telemetry/rte_telemetry.h | 50 +++++++++++++++++++++++++++++
> lib/telemetry/telemetry_data.c | 58 ++++++++++++++++++++++++++++++++++
> lib/telemetry/version.map | 9 ++++++
> 3 files changed, 117 insertions(+)
>
> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index 40e9a3bf9d..88b34097b0 100644
> --- a/lib/telemetry/rte_telemetry.h
> +++ b/lib/telemetry/rte_telemetry.h
> @@ -10,6 +10,7 @@ extern "C" {
> #endif
>
> #include <stdint.h>
> +#include <rte_compat.h>
>
> /** Maximum length for string used in object. */
> #define RTE_TEL_MAX_STRING_LEN 128
> @@ -20,6 +21,11 @@ extern "C" {
> /** Maximum number of array entries. */
> #define RTE_TEL_MAX_ARRAY_ENTRIES 512
>
> +#define RTE_TEL_U8_BITS 8
> +#define RTE_TEL_U16_BITS 16
> +#define RTE_TEL_U32_BITS 32
> +#define RTE_TEL_U64_BITS 64
> +
Not sure these are really necessary, but fairly harmless I suppose.
> /**
> * @file
> *
> @@ -153,6 +159,27 @@ int
> rte_tel_data_add_array_container(struct rte_tel_data *d,
> struct rte_tel_data *val, int keep);
>
> +/**
> + * Convert a unsigned integer to hexadecimal encoded strings and add this string
> + * to an array.
> + * The array must have been started by rte_tel_data_start_array() with
> + * RTE_TEL_STRING_VAL as the type parameter.
> + *
> + * @param d
> + * The data structure passed to the callback
> + * @param val
> + * The number to be returned in the array as a hexadecimal encoded strings.
> + * The type of ''val' can be one of uint8_t, uint16_t, uint32_t and uint64_t.
Not sure this last line needs to be stated.
> + * @param val_bits
> + * The total bits of the input 'val'. If val_bits is zero, the value is stored
> + * in the array as hexadecimal encoded string without padding zero.
> + * @return
> + * 0 on success, negative errno on error
> + */
> +__rte_experimental
> +int rte_tel_data_add_array_uint_hex(struct rte_tel_data *d, uint64_t val,
> + uint16_t val_bits);
> +
Just watch for whitespace consistency and coding standards. The "int"
should be on a line by itself, so the function name always starts in
column 0 of a line.
I would also suggest renaming "val_bits" - maybe "display_bitwidth" would
be clearer, though also rather long.
> /**
> * Add a string value to a dictionary.
> * The dict must have been started by rte_tel_data_start_dict().
> @@ -231,6 +258,29 @@ int
> rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> struct rte_tel_data *val, int keep);
>
> +/**
> + * Convert a unsigned integer to hexadecimal encoded strings and add this string
> + * to an 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 of the value is to be stored in the dict
> + * Must contain only alphanumeric characters or the symbols: '_' or '/'
> + * @param val
> + * The number to be stored in the dict as a hexadecimal encoded strings.
> + * The type of ''val' can be one of uint8_t, uint16_t, uint32_t and uint64_t.
> + * @param val_bits
> + * The total bits of the input 'val'. If val_bits is zero, the value is stored
> + * in the array as hexadecimal encoded string without padding zero.
> + * @return
> + * 0 on success, negative errno on error
> + */
> +__rte_experimental
> +int rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const char *name,
> + uint64_t val, uint16_t val_bits);
> +
same comments as above.
> /**
> * 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_data.c b/lib/telemetry/telemetry_data.c
> index 080d99aec9..fb2f711d99 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -4,6 +4,7 @@
>
> #include <errno.h>
> #include <stdlib.h>
> +#include <inttypes.h>
>
> #undef RTE_USE_LIBBSD
> #include <stdbool.h>
> @@ -12,6 +13,9 @@
>
> #include "telemetry_data.h"
>
> +#define RTE_TEL_UINT_HEX_STRING_BUFFER_LEN 64
> +#define RTE_TEL_UINT_HEX_FORMAT_LEN 16
> +
> int
> rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> {
> @@ -113,6 +117,33 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
> return 0;
> }
>
> +int
> +rte_tel_data_add_array_uint_hex(struct rte_tel_data *d, uint64_t val,
> + uint16_t val_bits)
> +{
> + char hex_str[RTE_TEL_UINT_HEX_STRING_BUFFER_LEN];
> +
> + switch (val_bits) {
> + case RTE_TEL_U8_BITS:
> + sprintf(hex_str, "0x%02"PRIx64"", val);
> + break;
> + case RTE_TEL_U16_BITS:
> + sprintf(hex_str, "0x%04"PRIx64"", val);
> + break;
> + case RTE_TEL_U32_BITS:
> + sprintf(hex_str, "0x%08"PRIx64"", val);
> + break;
> + case RTE_TEL_U64_BITS:
> + sprintf(hex_str, "0x%016"PRIx64"", val);
> + break;
> + default:
> + sprintf(hex_str, "0x%"PRIx64"", val);
> + break;
> + }
> +
> + return rte_tel_data_add_array_string(d, hex_str);
> +}
> +
This assume we only want those power-of-2 sizes. Is there a reason why we
can't use the code suggested by Morten in the discussion on v3? Having the
extra flexibility might be nice if we can get it.
If we do need to limit to only 8/16/32/64, then I recommend using an enum
in the header rather than #defines for those values. That makes it clearer
that only a very limited range is supported.
Also, code above treats all values other than 8/16/32/64 as if they were 0.
If 20, for example, is passed, we probably want to return error rather than
treating it as zero.
> static bool
> valid_name(const char *name)
> {
> @@ -220,6 +251,33 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> }
>
> +int
> +rte_tel_data_add_dict_uint_hex(struct rte_tel_data *d, const char *name,
> + uint64_t val, uint16_t val_bits)
> +{
> + char hex_str[RTE_TEL_UINT_HEX_STRING_BUFFER_LEN];
> +
> + switch (val_bits) {
> + case RTE_TEL_U8_BITS:
> + sprintf(hex_str, "0x%02"PRIx64"", val);
> + break;
> + case RTE_TEL_U16_BITS:
> + sprintf(hex_str, "0x%04"PRIx64"", val);
> + break;
> + case RTE_TEL_U32_BITS:
> + sprintf(hex_str, "0x%08"PRIx64"", val);
> + break;
> + case RTE_TEL_U64_BITS:
> + sprintf(hex_str, "0x%016"PRIx64"", val);
> + break;
> + default:
> + sprintf(hex_str, "0x%"PRIx64"", val);
> + break;
> + }
> +
I think the code for converting to hex should be in a common function used
by both the array and dict add functions, rather than duplicated as here.
> + return rte_tel_data_add_dict_string(d, name, hex_str);
> +}
> +
> struct rte_tel_data *
> rte_tel_data_alloc(void)
> {
> diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> index 9794f9ea20..951bd63974 100644
> --- a/lib/telemetry/version.map
> +++ b/lib/telemetry/version.map
> @@ -1,3 +1,12 @@
> +EXPERIMENTAL {
> + global:
> +
> + rte_tel_data_add_array_uint_hex;
> + rte_tel_data_add_dict_uint_hex;
> +
> + local: *;
> +};
> +
> DPDK_23 {
> global:
>
> --
> 2.33.0
>
next prev parent reply other threads:[~2022-12-13 17:12 UTC|newest]
Thread overview: 122+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-08 8:05 [PATCH 0/8] fix possible data truncation and conversion error Huisong Li
2022-12-08 8:05 ` [PATCH 1/8] telemetry: move to header to controllable range Huisong Li
2022-12-08 8:05 ` [PATCH 2/8] telemetry: add u32 telemetry data type API Huisong Li
2022-12-08 8:05 ` [PATCH 3/8] test: add test cases for u32 telemetry data API Huisong Li
2022-12-08 8:05 ` [PATCH 4/8] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-08 8:05 ` [PATCH 5/8] mempool: " Huisong Li
2022-12-08 8:05 ` [PATCH 6/8] cryptodev: fix possible data " Huisong Li
2022-12-08 8:05 ` [PATCH 7/8] mem: possible data truncation and " Huisong Li
2022-12-08 8:05 ` [PATCH 8/8] ethdev: telemetry convert capability related variable to hex Huisong Li
2022-12-08 10:55 ` Morten Brørup
2022-12-08 11:20 ` Bruce Richardson
2022-12-09 3:07 ` lihuisong (C)
2022-12-09 11:04 ` [PATCH V2 00/11] telemetry: add u32 value type and hex integer string API Huisong Li
2022-12-09 11:04 ` [PATCH V2 01/11] telemetry: move to header to controllable range Huisong Li
2022-12-09 11:04 ` [PATCH V2 02/11] telemetry: add u32 value type Huisong Li
2022-12-09 11:04 ` [PATCH V2 03/11] test: add test cases for adding u32 value API Huisong Li
2022-12-09 11:04 ` [PATCH V2 04/11] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-09 11:04 ` [PATCH V2 05/11] mempool: " Huisong Li
2022-12-09 11:04 ` [PATCH V2 06/11] cryptodev: fix possible data " Huisong Li
2022-12-09 11:04 ` [PATCH V2 07/11] mem: possible data truncation and " Huisong Li
2022-12-09 11:04 ` [PATCH V2 08/11] telemetry: refactor mapping betwween value and array type Huisong Li
2022-12-09 11:04 ` [PATCH V2 09/11] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-09 11:04 ` [PATCH V2 10/11] test: add test cases for adding hex integer values API Huisong Li
2022-12-09 11:04 ` [PATCH V2 11/11] ethdev: display capability values in hexadecimal format Huisong Li
2022-12-09 18:24 ` [PATCH V2 00/11] telemetry: add u32 value type and hex integer string API Morten Brørup
2022-12-12 6:23 ` lihuisong (C)
2022-12-11 9:02 ` fengchengwen
2022-12-12 6:42 ` [PATCH V3 " Huisong Li
2022-12-12 6:42 ` [PATCH V3 01/11] telemetry: move to header to controllable range Huisong Li
2022-12-12 6:42 ` [PATCH V3 02/11] telemetry: add u32 value type Huisong Li
2022-12-12 6:42 ` [PATCH V3 03/11] test: add test cases for adding u32 value API Huisong Li
2022-12-12 6:42 ` [PATCH V3 04/11] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-12 6:43 ` [PATCH V3 05/11] mempool: " Huisong Li
2022-12-12 6:43 ` [PATCH V3 06/11] cryptodev: fix possible data " Huisong Li
2022-12-12 6:43 ` [PATCH V3 07/11] mem: possible data truncation and " Huisong Li
2022-12-12 6:43 ` [PATCH V3 08/11] telemetry: refactor mapping betwween value and array type Huisong Li
2022-12-12 6:43 ` [PATCH V3 09/11] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-12 6:43 ` [PATCH V3 10/11] test: add test cases for adding hex integer values API Huisong Li
2022-12-12 6:43 ` [PATCH V3 11/11] ethdev: display capability values in hexadecimal format Huisong Li
2022-12-12 10:31 ` [PATCH V3 00/11] telemetry: add u32 value type and hex integer string API Bruce Richardson
2022-12-12 11:02 ` Morten Brørup
2022-12-12 11:20 ` Bruce Richardson
2022-12-12 12:03 ` Morten Brørup
2022-12-12 12:16 ` Bruce Richardson
2022-12-13 11:00 ` Morten Brørup
2022-12-13 17:12 ` Bruce Richardson
2022-12-13 3:02 ` lihuisong (C)
2022-12-13 10:15 ` [PATCH V4 0/9] telemetry: fix data truncation and conversion error and add hex integer API Huisong Li
2022-12-13 10:15 ` [PATCH V4 1/9] telemetry: move to header to controllable range Huisong Li
2022-12-13 17:10 ` Bruce Richardson
2022-12-14 2:44 ` lihuisong (C)
2022-12-13 10:15 ` [PATCH V4 2/9] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-13 10:15 ` [PATCH V4 3/9] mempool: " Huisong Li
2022-12-13 10:15 ` [PATCH V4 4/9] cryptodev: fix possible data " Huisong Li
2022-12-13 10:15 ` [PATCH V4 5/9] mem: possible data truncation and " Huisong Li
2022-12-13 10:15 ` [PATCH V4 6/9] telemetry: refactor mapping between value and array type Huisong Li
2022-12-13 16:57 ` Bruce Richardson
2022-12-14 2:46 ` lihuisong (C)
2022-12-13 10:15 ` [PATCH V4 7/9] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-13 17:09 ` Bruce Richardson [this message]
2022-12-14 2:44 ` lihuisong (C)
2022-12-14 7:28 ` Morten Brørup
2022-12-14 12:17 ` lihuisong (C)
2022-12-13 10:15 ` [PATCH V4 8/9] test: add test cases for adding hex integer value API Huisong Li
2022-12-13 10:15 ` [PATCH V4 9/9] ethdev: display capability values in hexadecimal format Huisong Li
2022-12-14 12:32 ` [PATCH V5 0/8] telemetry: fix data truncation and conversion error and add hex integer API Huisong Li
2022-12-14 12:32 ` [PATCH V5 1/8] telemetry: move to header to controllable range Huisong Li
2022-12-14 12:32 ` [PATCH V5 2/8] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-14 12:32 ` [PATCH V5 3/8] mempool: " Huisong Li
2022-12-14 12:32 ` [PATCH V5 4/8] cryptodev: fix possible data " Huisong Li
2022-12-14 12:32 ` [PATCH V5 5/8] mem: possible data truncation and " Huisong Li
2022-12-14 13:00 ` Morten Brørup
2022-12-15 1:11 ` lihuisong (C)
2022-12-15 7:04 ` Morten Brørup
2022-12-15 7:56 ` lihuisong (C)
2022-12-14 12:32 ` [PATCH V5 6/8] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-14 15:38 ` Bruce Richardson
2022-12-15 1:32 ` lihuisong (C)
2022-12-14 12:32 ` [PATCH V5 7/8] test: add test cases for adding hex integer value API Huisong Li
2022-12-14 12:32 ` [PATCH V5 8/8] ethdev: display capability values in hexadecimal format Huisong Li
2022-12-15 10:31 ` [PATCH V6 0/8] telemetry: fix data truncation and conversion error and add hex integer API Huisong Li
2022-12-15 10:31 ` [PATCH V6 1/8] telemetry: move to header to controllable range Huisong Li
2022-12-15 10:31 ` [PATCH V6 2/8] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-15 10:31 ` [PATCH V6 3/8] mempool: " Huisong Li
2022-12-15 10:31 ` [PATCH V6 4/8] cryptodev: fix possible data " Huisong Li
2022-12-15 10:31 ` [PATCH V6 5/8] mem: possible data truncation and " Huisong Li
2022-12-15 10:31 ` [PATCH V6 6/8] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-15 10:46 ` Bruce Richardson
2022-12-15 11:27 ` lihuisong (C)
2022-12-15 12:00 ` Morten Brørup
2022-12-15 12:15 ` Bruce Richardson
2022-12-15 12:24 ` Morten Brørup
2022-12-15 12:45 ` lihuisong (C)
2022-12-15 12:52 ` Morten Brørup
2022-12-15 13:08 ` Bruce Richardson
2022-12-16 1:15 ` lihuisong (C)
2022-12-15 10:31 ` [PATCH V6 7/8] test: add test cases for adding hex integer value API Huisong Li
2022-12-15 10:31 ` [PATCH V6 8/8] ethdev: display capability values in hexadecimal format Huisong Li
2022-12-16 1:54 ` [PATCH V7 0/8] telemetry: fix data truncation and conversion error and add hex integer API Huisong Li
2022-12-16 1:54 ` [PATCH V7 1/8] telemetry: move to header to controllable range Huisong Li
2022-12-16 1:54 ` [PATCH V7 2/8] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-16 1:54 ` [PATCH V7 3/8] mempool: " Huisong Li
2022-12-16 1:54 ` [PATCH V7 4/8] cryptodev: fix possible data " Huisong Li
2022-12-16 1:54 ` [PATCH V7 5/8] mem: possible data truncation and " Huisong Li
2022-12-16 1:54 ` [PATCH V7 6/8] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-16 1:54 ` [PATCH V7 7/8] test: add test cases for adding hex integer value API Huisong Li
2022-12-16 9:31 ` Bruce Richardson
2022-12-19 7:04 ` lihuisong (C)
2022-12-16 1:54 ` [PATCH V7 8/8] ethdev: display capability values in hexadecimal format Huisong Li
2022-12-19 7:06 ` [PATCH V8 0/8] telemetry: fix data truncation and conversion error and add hex integer API Huisong Li
2022-12-19 7:06 ` [PATCH V8 1/8] telemetry: move to header to controllable range Huisong Li
2022-12-19 7:06 ` [PATCH V8 2/8] ethdev: fix possible data truncation and conversion error Huisong Li
2022-12-19 7:06 ` [PATCH V8 3/8] mempool: " Huisong Li
2022-12-19 7:06 ` [PATCH V8 4/8] cryptodev: fix possible data " Huisong Li
2022-12-19 7:06 ` [PATCH V8 5/8] mem: possible data truncation and " Huisong Li
2022-12-19 7:06 ` [PATCH V8 6/8] telemetry: support adding integer value as hexadecimal Huisong Li
2022-12-19 9:19 ` Bruce Richardson
2022-12-19 7:06 ` [PATCH V8 7/8] test: add test cases for adding hex integer value API Huisong Li
2022-12-19 7:06 ` [PATCH V8 8/8] ethdev: display capability values in hexadecimal format Huisong Li
2023-01-16 12:06 ` [PATCH V8 0/8] telemetry: fix data truncation and conversion error and add hex integer API lihuisong (C)
2023-01-30 10:39 ` lihuisong (C)
2023-02-05 20:43 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y5ixtryyO/0u2nym@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=huangdaode@huawei.com \
--cc=lihuisong@huawei.com \
--cc=liudongdong3@huawei.com \
--cc=mb@smartsharesystems.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).