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 8AD58A0543; Fri, 16 Dec 2022 02:15:48 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2C26140695; Fri, 16 Dec 2022 02:15:48 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 63B4640685 for ; Fri, 16 Dec 2022 02:15:46 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.56]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4NYB3D3dBlzJqGy; Fri, 16 Dec 2022 09:14:48 +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; Fri, 16 Dec 2022 09:15:43 +0800 Message-ID: Date: Fri, 16 Dec 2022 09:15:43 +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: Bruce Richardson , =?UTF-8?Q?Morten_Br=c3=b8rup?= 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> <98CBD80474FA8B44BF855DF32C47DC35D875AF@smartserver.smartshare.dk> From: "lihuisong (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) 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 21:08, Bruce Richardson 写道: > On Thu, Dec 15, 2022 at 01:52:02PM +0100, Morten Brørup wrote: >>> From: lihuisong (C) [mailto:lihuisong@huawei.com] >>> Sent: Thursday, 15 December 2022 13.46 >>> >>> 在 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? >> I had the same concern, so I looked it up. >> >> snprintf() returns the length that the string would have, regardless if it was truncated or not. >> >> In other words: >> >> If the string is truncated, snprintf() returns a value greater than the buffer length parameter given to it. >> >> It can be checked like this: >> >> if (snprintf(buf, len, "0x%" PRIx64, val) > len) >> return -EINVAL; >> >> In my opinion, checking for negative return values from snprintf() is not necessary. >> > +1 > One nit, because of the \0, we need to check for ">=" len since a return val > equal to length means the last character was truncated to make room for the > \0. ok, do this in this way.