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 BF6A2A0545; Thu, 15 Dec 2022 13:16:09 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5DF2640684; Thu, 15 Dec 2022 13:16:09 +0100 (CET) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id D566A40223 for ; Thu, 15 Dec 2022 13:16:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1671106568; x=1702642568; h=date:from:to:cc:subject:message-id:references: content-transfer-encoding:in-reply-to:mime-version; bh=MYw21I1sypq2rGeChLnSJnCeFLdoT1jQr/2EYL2ppNs=; b=gJbFtOt3yw3vPcRPlu1YR7ZEyeTJMTi803/iWAErATEQGensjwFztPdU uqU1Sivb6Rh3F2IFxk/314hEtwVQMFWoRhkCQ8vjZDjZcNy3EFCYt6mdW WEWlgqcgDeKdcZR1+MK+B9wllvHynNIbfZ7dluHnrqNyo2Atx60paN0SA VtAQhFj+J3uEGqCn17dhQfahLxPr8D35iCZgaeechrtaLroxMdZqsJPP0 lwsHfTO60PDy3T/SFjd3lVn8NIO6uLHA7rD5YZnDDJpQT7IjCGsrypqn2 Lhje2vizGNUBxALeFzXkZ/WuZys5Re0bXunaYQiXubePWD6QwyYo9y7Jf A==; X-IronPort-AV: E=McAfee;i="6500,9779,10561"; a="316298281" X-IronPort-AV: E=Sophos;i="5.96,247,1665471600"; d="scan'208";a="316298281" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Dec 2022 04:16:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10561"; a="756316921" X-IronPort-AV: E=Sophos;i="5.96,247,1665471600"; d="scan'208";a="756316921" Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmsmga002.fm.intel.com with ESMTP; 15 Dec 2022 04:16:06 -0800 Received: from orsmsx602.amr.corp.intel.com (10.22.229.15) by ORSMSX603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Thu, 15 Dec 2022 04:16:06 -0800 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16 via Frontend Transport; Thu, 15 Dec 2022 04:16:06 -0800 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.105) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.16; Thu, 15 Dec 2022 04:16:05 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=D+n/y5pN7aTSvFtfQGFWqsmFTQc8mYCIySj+C60+LfNPaZLS+AY72hSsnwRBlBvRDJwLXkO0chjrjfywnzXEqYaqx+Z9zS671EBaj/r5cqcV5aukL9Xx/dxPagxMnV3XS76k/HKaFrsFr/lfv3rP4cokl4pwnKEei4DuEZC2y5u/Hd4luZgEdi11kPxlYjqSnZIgkV7WjzCNdZVm+uds1Ju0Nx7pudmjjpe/c0nkHOPsTGQVEfkXcvdamXexuIf4VAknk9t+fSpMYMfJYfpeMgo2Q+A2IMEOJA3urPW2irXtRn2tjuzWoftzMDoblGmLPyH8ZG3SoINCPEyUbF7F4w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=zBZ65t3x/IgYnyB9BOH0L0O9q4bSwfwZ8pRXyqyXJFs=; b=EafUqIrDuFglhvMmW20OSvi45v9aRzHTViu/eTasDco+AQPuHYlcy1xjls7/7Z9aqbbpaE7zH3BmvwP/W/CdrDwsmw8nEb7xdMHZn+qNn9WbhWpwoXG7NDtGowcHyPjszkvnmyv5K4CCkoHBMWJH0oSPfQkq793Ti5wITlVitTcaZiA3qNoN4lL7rR5ZPiIjVI4heJNSgMl/BWrN0eBJbp/O9QJQ/3FpUTg4Q44/eY/TX+OhlcJPJy5e8wseDOI9ge1uWZcHE2qWxBzrmqQrCobI7laH4UXlb+fQg9PY0s4Uc3aFuLaZmijyNI7v6CDwM8LCpBL8WQN3P3Qozftogw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; Received: from MWHPR11MB1629.namprd11.prod.outlook.com (2603:10b6:301:d::21) by PH7PR11MB6674.namprd11.prod.outlook.com (2603:10b6:510:1ac::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5924.12; Thu, 15 Dec 2022 12:15:59 +0000 Received: from MWHPR11MB1629.namprd11.prod.outlook.com ([fe80::18bd:edae:ad31:a228]) by MWHPR11MB1629.namprd11.prod.outlook.com ([fe80::18bd:edae:ad31:a228%3]) with mapi id 15.20.5924.011; Thu, 15 Dec 2022 12:15:59 +0000 Date: Thu, 15 Dec 2022 12:15:51 +0000 From: Bruce Richardson To: Morten =?iso-8859-1?Q?Br=F8rup?= CC: "lihuisong (C)" , , , , , Subject: Re: [PATCH V6 6/8] telemetry: support adding integer value as hexadecimal Message-ID: 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> Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D875AD@smartserver.smartshare.dk> X-ClientProxiedBy: LO4P265CA0088.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:2bc::7) To MWHPR11MB1629.namprd11.prod.outlook.com (2603:10b6:301:d::21) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: MWHPR11MB1629:EE_|PH7PR11MB6674:EE_ X-MS-Office365-Filtering-Correlation-Id: 097b1170-273a-44ca-0acc-08dade962018 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 4K5SdNtlfMIFfpmszSQ8q+QsZLeqH2zl00G+sqyxt2U9KcGfwo94QsV1RNvB6/u2eZoP5AvfVRsakZ1Lfu84dFZcCNrb4kFBhks7aSAgvXP9IkQaUh+T+8E4351P5cJ8fHgrRtIxw2YydrnNrb8FBFzfjvtdDGhBtvbphIYKV47WuscmshvScZCbmkv5SAB8hp1G5YlykLrf1hs6inYi2paaAwbP8Jt0TbPk0LqvL88SHS4F9JGHMySibnEMBBmbKbEhybsudXJxPCm9eZ/GuGacbb12aOzcloAfKGLGSbnYbz7eL3Zy2aMoRu3QReNr6zmqFWpfh/B7912AmMCANq5Ho77OrYCKgjo5VfibvJkimvNU93LL/9s9RifS0dLiqX1m45XpSSppBduH8sIuZUt3jHMhkDBv/BcUkfOw7LpewnXl+EVolwKzZV4XKwFtl+irKpfIo+y9p1oNJtZZfUGh0QjoBbvNY/qThOLUjWlWQ06/Ck/+0HF2bBnF3svBfn+BiqXN9JjWfCz/nMdG54DWBR1sbSIe49w3UOAupHgktIOHe6ij1odWkGcvAbM//bJxGklMbLwiQ1WkD0eDGXYlsfkPFBI2Le83mWAYEX62CJn93a2Nmnx6plLWvOa53Dj7K9QNsai6quXjQyKigw== X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MWHPR11MB1629.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(366004)(376002)(346002)(39860400002)(136003)(396003)(451199015)(41300700001)(8936002)(6506007)(186003)(26005)(6512007)(2906002)(86362001)(66556008)(5660300002)(66946007)(4326008)(44832011)(38100700002)(82960400001)(8676002)(66476007)(83380400001)(316002)(66574015)(6916009)(6666004)(6486002)(478600001); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ajBXOWRFNjhvZXJ4NWRLQmd2ZVRXclRHQkd3VldLdFBUZTBZOTgvdnYrU29x?= =?utf-8?B?eDRwMHB1WEFCWHloL1BlSVhDUnRGUEtoOHRrU0hWbE95Tm9KcldvdkdPaWds?= =?utf-8?B?RjVUSWNLdUw0VFBERnA4T2k4aTdSWWM4WmFTckJnekRVRTJrbUF0bVVkSTZZ?= =?utf-8?B?VStrVFJaZEFlaDFCOVQrS29lTEIzbzFjak5lazZjbmVJNGpIejk5LzVnczYr?= =?utf-8?B?NzIyZVdkbDRsV21SVjZoVGExc0RxWEpLN1JrYjl2aHNheUdLQ0hXcWpvS0kx?= =?utf-8?B?Zko5bHh5a0x5Q1dPeldsa01zc3Z6RUlEbmorWWpTS002K1lyUU5EUExOcmgv?= =?utf-8?B?VWFKS2xESEhva0pRUFZNQ3pnOTlUbzk4WGljMSszeldTSjBsbTlWR1RZR2Rs?= =?utf-8?B?YWlEaWFxN09SZjl5N1VjdEM0Z3lzNzUwbVFHZlROQklpUUlId2ZkTDQyWnYw?= =?utf-8?B?bEFBYnJTdlpCZjQwaEZHL3IwaGJtOGl2OFlPVHhZSUZpSGhtSHJ3RUdPNDh3?= =?utf-8?B?QnN6Z1dWalZhZHZDYis5ZjJ2WVFKb3c4ZzFUUE1VQW90YStWRHVKVHpFVVdE?= =?utf-8?B?bTE4aEU5R1loT08reDRkRXk0ZXVDV3ZZL3RMM3NkZG9vcHVpai8yTnorRUcr?= =?utf-8?B?c2ZCRGY0U3NZQ0JCMFFjU3hVNzMxK1BvVko0R25yd0FWUnNUWmRZN2toNGZz?= =?utf-8?B?bU52R2t6U1lTdk1SdlhaUVJqcDV1UFN6SmcvZDZ3cjZjRUIrQVF3SzQzSzl6?= =?utf-8?B?Y2tjNWQ2U05pV08xSlpEUkpURUxISTlvRUJKa3RGa0xoRlpDY2d1R0E3cHpX?= =?utf-8?B?QmVhdnN4YUVFcGIzRFFHTTNtVmNjTHkxNmR0ZmdBMlpWZnU1dkNzMk5iejVq?= =?utf-8?B?YmF2aUhockxub2p6Y1MzZEdGR0VUVUVJbUdhRmVKNmFOZHg3ZEhlUTBSQnM0?= =?utf-8?B?bDVFbU9jQ0lFczhLVjJOZ2hJbm1CRDlqcGtUb3l5U1FPdy8ybmYvYlp6cjlH?= =?utf-8?B?cDFMY0E2QUkwR2RKd1hvdDFWazFqVmpNSHgySzY2cmtxeXlOU3FoUGtrd0wx?= =?utf-8?B?YlFCZ2dLbmdKZHZqOXcrczN2V3lFbTFueDBwTjdPWE5CNWpVeGxjNG9IUkxI?= =?utf-8?B?ZUx2UWNHbVltRk9kaEZNNlh6a2FmdEMxRzJkNHJZS0ptOEovU1NWdkJLdm9P?= =?utf-8?B?cytRdk1oVkJpVHUzR1BydkNwYmJvaEVTSEFjRWlRYVFZU2J3ZE5zYVpUekMv?= =?utf-8?B?M1paUDRrNy9rcWkxK0R2NVl5Njk5SEw5MjNybGJ3YmI0YU9MZE1JS3REaUV2?= =?utf-8?B?ZXdDSGhqeUN1OWpRaVkrTTE5TmdqTnFnU291TW5RQ3JCVVgxcUF2RVVjLzUr?= =?utf-8?B?eFFjVTJmN1lwZEhLMnk5SVhvWXZMUDhjcjdtYUVmRERQbThoWUxxM3YrOHBO?= =?utf-8?B?NjlMUnAyOHpPQlFtNWF4M09ISFdZQmNCQnBUVzZHTU5PMzR6QUNDTlhnU2xO?= =?utf-8?B?MUxGT2gwR0ZEb05rVTBqdEJqSFMyNVA2eHNjTUpicER3WkdQc2VzTVJYT3Fx?= =?utf-8?B?cmtkSENkYjY5NmQ3ckV6WjBPTWVtQ0s5b3FheVhGc0xibFJPN1VubXcrei9S?= =?utf-8?B?S1ZaSGduenR0ZUEzSXlzRzZQM0RNU0JWdG5DWnI5YzV6aFBrVC9qY3RQYXhh?= =?utf-8?B?L3dzT3FIN0w3WjVYY1hlSTVDYVJnejBvNDBWRkNtZS9mUUk1NmxrQzJUSnRL?= =?utf-8?B?a0txQUU2ZVhjVkEwUmlwNy9qdFdhV012d3BlKzJiam56SGFSQkJ3NkY1aTE4?= =?utf-8?B?MWFpd1IxbVVLeFh5WUpGcWwyZDJwUUdBb3VJVStEYVdSaW9sdE5LOGp5N1BN?= =?utf-8?B?TFJTZjFHY21CODk0QjFpRjgyN3A4T21FbWluOVhTeStrYU1wWTh4Nk1DcjdU?= =?utf-8?B?SHBhWG5QWHd5UjNXalgxendNRzREaFRlWVlNc1NsUng0cUFnaXlqUTVBNldR?= =?utf-8?B?WkV0dEtHWmcyazJ0ZDE3ZGZwMXFNU2t5UE9KVTk2NXA0ZFZIS0ZnTmFtOFkw?= =?utf-8?B?N0tONW1SNWxBVnorUmhNRERLS3FzZWwrVmZIS1lML1ZnYlVMbFNXaXBEQlMv?= =?utf-8?B?Ny9scmFScnVtOE5PcGsrZjFUMVRqTjcvSVRrUXNmUkQ3NVNzRVQ5UnNvUm9B?= =?utf-8?B?QWc9PQ==?= X-MS-Exchange-CrossTenant-Network-Message-Id: 097b1170-273a-44ca-0acc-08dade962018 X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB1629.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2022 12:15:59.2816 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: +Yujp6Rvikbl0wxt63expLU9zEi3kgZ1Adqatzd9uD80nkMWGO2JtNo8SFIcfQPSeBFMPZGWTB0w4kE+cuikqr0ur3x1e/uRvEXV8KlPpOg= X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6674 X-OriginatorOrg: intel.com 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 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. /Bruce