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 34839A0543; Wed, 14 Dec 2022 13:17:31 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C7330400D6; Wed, 14 Dec 2022 13:17:30 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 7FE924003F for ; Wed, 14 Dec 2022 13:17:28 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4NXDqg0LqrzFprd; Wed, 14 Dec 2022 20:16:31 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Wed, 14 Dec 2022 20:17:25 +0800 Message-ID: <8204812e-0194-33b6-6839-59c48032f439@huawei.com> Date: Wed, 14 Dec 2022 20:17:24 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH V4 7/9] telemetry: support adding integer value as hexadecimal To: =?UTF-8?Q?Morten_Br=c3=b8rup?= , Bruce Richardson CC: , , , , References: <20221208080540.62913-1-lihuisong@huawei.com> <20221213101512.39919-1-lihuisong@huawei.com> <20221213101512.39919-8-lihuisong@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35D87596@smartserver.smartshare.dk> From: "lihuisong (C)" In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87596@smartserver.smartshare.dk> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected 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 在 2022/12/14 15:28, Morten Brørup 写道: >> From: lihuisong (C) [mailto:lihuisong@huawei.com] >> Sent: Wednesday, 14 December 2022 03.44 >> >> 在 2022/12/14 1:09, Bruce Richardson 写道: >>> 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 >>>> Acked-by: Morten Brørup >>>> Acked-by: Chengwen Feng >>> 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 >>>> +#include >>>> >>>> /** 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. >> This is convenient for the user to use. >>>> /** >>>> * @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. >> Sorry, I refer to a wrong example. >>> I would also suggest renaming "val_bits" - maybe "display_bitwidth" >> would >>> be clearer, though also rather long. >> The 'val_bits' means the total bits of input 'val', It also reflects >> the >> type of data >> to be stored in hexadecimal. After all, we use 'u64' to cover all >> unisgned integer types. >> And this function is introduced for adding hexadecimal format value, >> not >> binary format. >> The value can not be stored exactly according to the input >> "display_bitwidth". >> If we limit to only 8/16/32/64 integer types, the 'val_bits' is better, >> I think. > NAK to limiting the bitwidth to 8/16/32/64 bits! > > The function should be able to dump bitfields, such as the TCP flags, where the bitwidth is 6. > >>>> /** >>>> * 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 >>>> #include >>>> +#include >>>> >>>> #undef RTE_USE_LIBBSD >>>> #include >>>> @@ -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. >> The compiler doesn't like it. There is a warning: >> 'warning: format not a string literal, argument types not checked >> [-Wformat-nonliteral]' > You can surround the affected functions by some #pragma to temporarily disable that warning. Good idea! I will fix it in V5. > > I assume you noticed the bugs in my code: > > char str[64]; // FIXME: Use correct size. > if (bits != 0) { > char format[16]; // FIXME: Use correct size. > sprintf(format, "0x%%0%u" PRIx64, (bits + 3) / 4); // bug fixed here > sprintf(str, format, value); > } else { > sprintf(str, "0x%" PRIx64, value); > } > > >>> 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. >> I have to only consider 8/16/32/64 integer types because of above >> warning. >> In addition, the normal unsigned integer data is one of them. If user >> forces >> '0xf1f23' value to 10 bitwidth to display, it will be truncated as >> 0xf23. > The printf width field specifies the MINIMUM number of characters to output. > > Truncation would be a bug in the C library. > >> It seems pointless and unfriendly. >> So overall, this function is limited to all uint types, and is >> currently >> fully adequate. >> >> Do we need to check other bitwidth? If the 'val_bits' isn't 8/16/32/64, >> it is processed as >> no-padding zero. >