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 37943A0545; Thu, 15 Dec 2022 12:28:00 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 23F7240684; Thu, 15 Dec 2022 12:28:00 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id AA2EC40223 for ; Thu, 15 Dec 2022 12:27:58 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.55]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4NXqh10VHPz16LdR; Thu, 15 Dec 2022 19:26:57 +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 19:27:56 +0800 Message-ID: <91c7e257-9c7c-849e-c768-cd48a6e659a3@huawei.com> Date: Thu, 15 Dec 2022 19:27:55 +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 CC: , , , , , References: <20221208080540.62913-1-lihuisong@huawei.com> <20221215103146.816-1-lihuisong@huawei.com> <20221215103146.816-7-lihuisong@huawei.com> 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 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, >> + 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; > >> + if (display_bitwidth != 0) { >> + sprintf(format, "0x%%0%u" PRIx64, spec_hex_width); >> + sprintf(buf, format, val); >> + } else { >> + sprintf(buf, "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? > > Thanks, > /Bruce > .