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 0FC48A0545; Thu, 15 Dec 2022 13:45:59 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B08BE40684; Thu, 15 Dec 2022 13:45:58 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 36AA140223 for ; Thu, 15 Dec 2022 13:45:56 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4NXsL80wMdzgZ4C; Thu, 15 Dec 2022 20:41:36 +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; Thu, 15 Dec 2022 20:45:54 +0800 Message-ID: Date: Thu, 15 Dec 2022 20:45:53 +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 V6 6/8] 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> <20221215103146.816-1-lihuisong@huawei.com> <20221215103146.816-7-lihuisong@huawei.com> <91c7e257-9c7c-849e-c768-cd48a6e659a3@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35D875AD@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35D875AE@smartserver.smartshare.dk> From: "lihuisong (C)" In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D875AE@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/15 20:24, Morten Brørup 写道: >> From: Bruce Richardson [mailto:bruce.richardson@intel.com] >> Sent: Thursday, 15 December 2022 13.16 >> >> On Thu, Dec 15, 2022 at 01:00:40PM +0100, Morten Brørup wrote: >>>> From: lihuisong (C) [mailto:lihuisong@huawei.com] >>>> Sent: Thursday, 15 December 2022 12.28 >>>> >>>> 在 2022/12/15 18:46, Bruce Richardson 写道: >>>>> On Thu, Dec 15, 2022 at 06:31:44PM +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 >>>> value >>>>>> as hexadecimal encoded string to array or dictionary. And user >> can >>>> choose >>>>>> whether the stored value is padding to the specified width. >>>>>> >>>>>> Signed-off-by: Huisong Li >>>>>> Acked-by: Morten Brørup >>>>>> Acked-by: Chengwen Feng >>>>>> --- >>>>>> lib/telemetry/rte_telemetry.h | 47 ++++++++++++++++++++++ >>>>>> lib/telemetry/telemetry_data.c | 72 >>>> ++++++++++++++++++++++++++++++++++ >>>>>> lib/telemetry/version.map | 9 +++++ >>>>>> 3 files changed, 128 insertions(+) >>>>>> >>>>> >>>>>> +/* To suppress compiler warning about format string. */ >>>>>> +#if defined(RTE_TOOLCHAIN_GCC) >>>>>> +#pragma GCC diagnostic push >>>>>> +#pragma GCC diagnostic ignored "-Wformat-nonliteral" >>>>>> +#elif defined(RTE_TOOLCHAIN_CLANG) >>>>>> +#pragma clang diagnostic push >>>>>> +#pragma clang diagnostic ignored "-Wformat-nonliteral" >>>>>> +#endif >>>>>> + >>>>>> +static int >>>>>> +rte_tel_uint_to_hex_encoded_str(char *buf, uint16_t len, >> uint64_t >>>> val, >>> The "len" parameter should be size_t or unsigned int, not uint16_t. >>> >>> And as a personal preference, I would name it "buf_len" instead of >> "len". >>>>>> + uint8_t display_bitwidth) >>>>>> +{ >>>>>> +#define RTE_TEL_UINT_HEX_FORMAT_LEN 16 >>>>>> + >>>>>> + uint8_t spec_hex_width = (display_bitwidth + 3) / 4; >>>>>> + char format[RTE_TEL_UINT_HEX_FORMAT_LEN]; >>>>>> + >>>>>> + /* Buffer needs to have room to contain the prefix '0x' and >> '\0'. >>>> */ >>>>>> + if (len < spec_hex_width + 3) >>>>>> + return -EINVAL; >>>>>> + >>>>> This check is not sufficient, as display_bitwidth could be 0 for >>>> example, >>>>> and the actual printed number have up to 16 characters. >>>> Yes, you are right. What do you think of the following check? >>>> >>>> max_hex_width = display_bitwidth == 0 ? (sizeof(uint64_t) * 2) : >>>> spec_hex_width; >>>> if (len < max_hex_width + 3) >>>>     return -EINVAL; >>> LGTM. >> That would work, but actually I think we should drop this check >> completely >> - see comment below. >> >>>>>> + if (display_bitwidth != 0) { >>>>>> + sprintf(format, "0x%%0%u" PRIx64, spec_hex_width); >>>>>> + sprintf(buf, format, val); >>> snprintf(format, sizeof(format), "0x%%0%u" PRIx64, spec_hex_width); >>> snprintf(buf, len, format, val); >>> >>>>>> + } else { >>>>>> + sprintf(buf, "0x%" PRIx64, val); >>> snprintf(buf, len, "0x%" PRIx64, val); >>> >>>>>> + } >>>>>> + >>>>> snprintf should always be used when printing to the buffer so as >> to >>>> avoid >>>>> overflow. The return value after snprintf should always be >> checked >>>> too. >>>> If check for buffer is sufficient, can the return value of snprintf >> not >>>> be checked? >>>> There are also many places in telemetry lib that are not checked >> for >>>> this return value. >>>> Do you have any principles for this? >>> You already check the buffer size before printf() into it, so I think >> it suffices. However, to keep automated code checkers happy, you could >> easily use snprintf() instead of printf(). (Sorry about not doing it in >> my example.) >>> I somewhat disagree with Bruce's suggestion to check the return value >> from snprintf() after checking that the buffer is large enough. It >> would be effectively dead code, which might cause some confusion. On >> the other hand, it might keep some automated code checkers happy. In >> this specific case here, I don't have a strong preference. >> Sure, you don't need 2 checks - we can either check the length at the >> start, or else check the length by looking for the return value from >> snprintf, but we don't need to do both. >> >> Given the slight complexity in determining the correct length of the >> printf >> for the initial size check, I think I would go with the approach of >> *not* >> checking the buffer initially and just check the snprintf return value. >> That would remove any possibility of us doing an incorrect length >> check, as >> was the case originally with this patch. > That sounds reasonable to me. Please do that. I think above check is necessary. Because snprintf returns the total length of string formated instead of negative when buffer size isn't sufficient. what do you think? /Huisong