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 5A04D428C6; Tue, 4 Apr 2023 18:28:45 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DEB8C40EE3; Tue, 4 Apr 2023 18:28:44 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id A27F640A7E for ; Tue, 4 Apr 2023 18:28:41 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1680625722; x=1712161722; h=date:from:to:cc:subject:message-id:references: in-reply-to:mime-version; bh=dhC/oQg6JjZMFGFSN4ydORsXnWWcbcQ42e8F68Q3h68=; b=CxzBYmhdPXA2E31/gTatGWWP9QUPRdlSCjNPpbxjiTfuFrKbdM8cNyWd ptF6EXs8meWfSkwPCprKctklLakMFOMXnRZELhuCfcf7j+9od56bu/wuw +hRDGfFPcKLbi/hlQrsmvSZWMNBH0SjG/XQiZ3srjYgdwIxh2cdmkb85T 2hTkd/nTxWv9YLP4/a0b+DZ9mjlF8ZLib3jPxW/yCZt7xjluBax5iUDkD /wh9hBffG7pKAJwUXhTpq9px26C1AKH8nJ80iTdHHqPywbG44sSR6FCA5 8fI0qX/LJe1SLtNgMrgE7iUIEd1qHimCSrgnPep8+bfTT7jfG/Gigrypa g==; X-IronPort-AV: E=McAfee;i="6600,9927,10670"; a="428523568" X-IronPort-AV: E=Sophos;i="5.98,318,1673942400"; d="scan'208";a="428523568" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Apr 2023 09:28:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10670"; a="775709155" X-IronPort-AV: E=Sophos;i="5.98,318,1673942400"; d="scan'208";a="775709155" Received: from orsmsx602.amr.corp.intel.com ([10.22.229.15]) by FMSMGA003.fm.intel.com with ESMTP; 04 Apr 2023 09:28:39 -0700 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) 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.21; Tue, 4 Apr 2023 09:28:38 -0700 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX610.amr.corp.intel.com (10.22.229.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Tue, 4 Apr 2023 09:28:38 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx610.amr.corp.intel.com (10.22.229.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21 via Frontend Transport; Tue, 4 Apr 2023 09:28:38 -0700 Received: from NAM02-SN1-obe.outbound.protection.outlook.com (104.47.57.47) 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.21; Tue, 4 Apr 2023 09:28:38 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QHYzjFELUQVDGVsTILYMki3ZakGdeYzqP4lIcyzgmGxMFrPFDXE/kjEWY34l0mEnZh4XCuCPM6cT1vZMfFgLVr7ebLb52wyb0Au7H1Z+q7yIkMvNX/EDlkRLupT+3BlTZdaRdWgbmbJcBrGClErZruanO1dqg3iIG64j27j3Tmazn+o/YVAXdh2Hdd/8YI32PCttfBTSKJfDskqs1Sk2JbQibPCb1TMXs+MS8WCzi/kT0lY64PDk4hUYumUgvEKUxSaSF9Yh4Rq12DYa2k0M8cocC2jY7MeweZe2uuNNtrk15bkL8I8nTww/TZDXYplJdTgu27VkhtayQehWVTZ9RQ== 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=DhaF/rSoFwfCXfXzo91XaCjoTX9g1CRRJJuxoAtSGtc=; b=Pa2g3cXS0KjpIv5OJRmV87RKJ4PvB8eAwr5Hs46IdI0kLBdj/fOu/xeT0nvBfrcF2g6CVlCP33x6V7U2MmaSgFWwnMwI+pEuQmzkvInshuNEGwMDsNqkR/WLJBYut/yRHL1V71EtcfjVKifw44hLxnBG+aeqPnrDbvb2vQtW5yaUsE57Uyr/yynj6wkYW9bXcS8QlneUKwVpp9z6lSYnCQVNMdEcgn3JA1l2ExxkDqlfVNivFmfLuNQdyJXOI8DyywAbbQBI7Q6AgIT+SmEH6eZj4I2bce6O1BA4Q816rFoVwPw0KIVlsuWsPgoDOTPOFdYgvQsMsoOKMu9I4wvtBA== 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 DS0PR11MB7309.namprd11.prod.outlook.com (2603:10b6:8:13e::17) by BN9PR11MB5545.namprd11.prod.outlook.com (2603:10b6:408:102::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6254.35; Tue, 4 Apr 2023 16:28:36 +0000 Received: from DS0PR11MB7309.namprd11.prod.outlook.com ([fe80::18d0:ac53:aa1d:d19c]) by DS0PR11MB7309.namprd11.prod.outlook.com ([fe80::18d0:ac53:aa1d:d19c%6]) with mapi id 15.20.6254.035; Tue, 4 Apr 2023 16:28:36 +0000 Date: Tue, 4 Apr 2023 17:28:29 +0100 From: Bruce Richardson To: Tyler Retzlaff CC: Stephen Hemminger , , , , Subject: Re: [PATCH 1/2] telemetry: use malloc instead of variable length array Message-ID: References: <1680539424-20255-1-git-send-email-roretzla@linux.microsoft.com> <1680539424-20255-2-git-send-email-roretzla@linux.microsoft.com> <20230403131913.0aec54ce@hermes.local> <20230404162444.GB18560@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20230404162444.GB18560@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-ClientProxiedBy: LO4P123CA0263.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:194::16) To DS0PR11MB7309.namprd11.prod.outlook.com (2603:10b6:8:13e::17) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DS0PR11MB7309:EE_|BN9PR11MB5545:EE_ X-MS-Office365-Filtering-Correlation-Id: 38f2f4a7-74c6-4b1f-7235-08db3529a393 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: GRme86Mx7DheP0ARRCxGAjE9vdBqHd+a82OtflOSloRMug5abkkU8Ph2zsBjIwL/Kb+4w0O3KGrT2KFuVyRv/W1MzOOT6l2oI+igUZ97i1tSviN5y5Ov+GlX8HtrC2RGY2h8C6PYF3J3UR2DJftjDWlAamW4UYLkl68Rwgcfn56BTZtC/WmarW/OW/x4qumFXNLYIG8aSTOFfhGaoXXu+DBA4aC7D+bZ7X053ntb4u2xyzZg1tcpozPw4Gx4m+h0t3nD2pl9O/8MgMry+134gD2w30ja2C/qBjrqT9ZD3FytmHDB7Igljn8HWlscSEtiLtOYRO3FSjeEBHN3j3zHhj05ChINaQ4xzMWDgz/q8HYsOKFbIglaFIFJ/djnNpDHgW3OgGJOd8qhoXx0MP1Ejc4OE3BxQE2GcrG4RuvJn8TnoFBO1TLbGHXOul7aEnfwx70uek9Ev6EJASrKIJ1T/ZmroLPrd7eFygDr4qRl1vG0yebgVRehqykRTAsc6nC0KsIkho2i2XVOCPNlSB5RyPdgxXVED7UoSBmUmwZtZ0hzs2HeSfSvuu/8EH9YLp0P X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DS0PR11MB7309.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230028)(396003)(376002)(346002)(366004)(39860400002)(136003)(451199021)(6666004)(6486002)(83380400001)(86362001)(38100700002)(82960400001)(6506007)(26005)(6512007)(186003)(316002)(2906002)(66946007)(66556008)(6916009)(44832011)(4326008)(8676002)(66476007)(8936002)(5660300002)(41300700001)(478600001); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?u2Bofe1SJPeoSfUMZOGXEHcAoxvA5s6S3yZUApl3ahwiyckxnkyV7HTZ7mAR?= =?us-ascii?Q?MHQ3hGqQ1O3LYT1jzSW3bfpTj2wK3egUN3+PQHAXA+YYhbUm2yujyhXj6Uxa?= =?us-ascii?Q?OhoLE8UqgnIVo6IZY4xaY+Hdjf+xPEAjZcfvu497C8Gnt+ejLXz121MSqM6+?= =?us-ascii?Q?WuFCPfVHidP0X1ljKcvhg+wz5bnzlbo3AyUfxKZ39KpIpVMnxqgWWu7lDLYt?= =?us-ascii?Q?1OoN6W6X+JBIJ/Ia5R1ieU6+9FSfIbQg+Rbk5eN3EkezGPmq5u7SHmdSzxy6?= =?us-ascii?Q?XF4PW/M/RHPw3vm6RPnLD6kTCWt9wAu80A6TkddzmThmFAfB/LF4OHIOVtId?= =?us-ascii?Q?U7/P1R7JERgt79FB2oQ77qkz+EqVNzRjElJdk2TAd4zcAKYUuSA4T9l32oPG?= =?us-ascii?Q?/rCN2r1yC6DsDSTTP4lPzyz3N0/yz7Qul3tfJgiz4kUi+K+3nvgvAh5g1FI4?= =?us-ascii?Q?riPe80Hhp92LiGHrMkyVeGyiZ0noned/usynDfO9WWN+Thr7lfutldeo1n0c?= =?us-ascii?Q?mBjxUJp6Dz5+3jAYn+0XPOmIPhu/FHfQlQ2zR3LFdEjwERHfD6FHkvy4TeKj?= =?us-ascii?Q?omKWYPuYkpPsnTA3zNXIKs089QzAEbLciSO3Oc4jhYanls1x8/JQ45+J8+3S?= =?us-ascii?Q?JTZV6/kd8OMGAE3mQyTZYnZiea3SbLAEgk8gd7Ty4txt7+gSLe+XpGNHUfJZ?= =?us-ascii?Q?j3ticqFaOXeO0lhdH+2EVn9G8dUp7ttBCB4wihJ8eCY3R/kIsQOL2wlFoXJx?= =?us-ascii?Q?NlkSw2+nWi/K1MLE6WrJ5UYKeWtFRDXkSKAqybvkjuY4WPK4+tqAo7bIQ2pi?= =?us-ascii?Q?MF73TgSbZ5wr/W3SternCYygmJSkTc8hWmr20YRUzAeYzSdp6gt8bmfrt20q?= =?us-ascii?Q?GYI1VLKBaiOA0sZUR9HlHUsIt+CsvacJtQfM7Rykpi5qI2EwFBg0eV6ozDGT?= =?us-ascii?Q?gZvXW4+G3OhbXUVNNPA3VG8muCB8Hfbnba6BS9t5U9H5ynGQmno9y+5NQ/n/?= =?us-ascii?Q?6TEFTYeFb8ihByyErJFhoYqgUFdMnaCXyaTQdikTy1UyWDwLwaw1fJil6Oao?= =?us-ascii?Q?/BcSPfWogwqoTL6DfmB6oHw0e5BJOb1EqLprTONNr87ipO1lluxumVMkzLTP?= =?us-ascii?Q?RlkQyDEc9MOfWADe2rJtUcLSrjzY1iY8PaQcMiNrJjf1tdrAnFXxrkWsDfYM?= =?us-ascii?Q?WeWcCZm0vSeDsqfkMDXkYkF+AOfa4pzYM4AVfgbl0YWUbqV9y8Dd0We8NEE3?= =?us-ascii?Q?SvGvTGp9oIDgeR8XK0VgJJYE2XNflMILFR2NuZbwcVHK7D6QZ0zWoT3eFE8+?= =?us-ascii?Q?YKlolxQ9Za/CFN83pTVeU6PeLpZb0bvzhwL3Q/ID0ypxfEOtiR1goX7WJdrk?= =?us-ascii?Q?RbXVcupWI+3dC7iLBSfavuiynJ2elaaJJnlvJut6py1JckzfhxKEbPhT4d0c?= =?us-ascii?Q?yn5Fxj0D71w06o4m99Y+fS7VurRVznZ0Rhb0CFmC2hfwiJtrus6wABtZiVA4?= =?us-ascii?Q?y6lCzbL5BF5jU8/oZZPFr5skH0wvOyo0eIqDtJuUaNl+VZFHc0HLRfi9yinh?= =?us-ascii?Q?kJcCCqAVnXz6vSa1HuIzhz8N1kD2Z1ITVltZ9EAJrvs+VP2b+l2rhrSS4lNr?= =?us-ascii?Q?rg=3D=3D?= X-MS-Exchange-CrossTenant-Network-Message-Id: 38f2f4a7-74c6-4b1f-7235-08db3529a393 X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7309.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2023 16:28:35.9024 (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: TmWLmsDwMZ+3mX3mAV5cAWqxsWTOMTG/4bOZA4mFYp04yUb5P7df4B8Nh7QUJxXq3erl3VVXL7GhwBg1m+UlRddHJiajuFmkdu7LlDdM54M= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5545 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 Tue, Apr 04, 2023 at 09:24:44AM -0700, Tyler Retzlaff wrote: > On Tue, Apr 04, 2023 at 09:47:21AM +0100, Bruce Richardson wrote: > > On Mon, Apr 03, 2023 at 01:19:12PM -0700, Stephen Hemminger wrote: > > > On Mon, 3 Apr 2023 09:30:23 -0700 > > > Tyler Retzlaff wrote: > > > > > > > __json_snprintf(char *buf, const int len, const char *format, ...) > > > > { > > > > - char tmp[len]; > > > > + char *tmp = malloc(len); > > > > va_list ap; > > > > - int ret; > > > > + int ret = 0; > > > > + > > > > + if (tmp == NULL) > > > > + return ret; > > > > > > > > va_start(ap, format); > > > > ret = vsnprintf(tmp, sizeof(tmp), format, ap); > > > > va_end(ap); > > > > if (ret > 0 && ret < (int)sizeof(tmp) && ret < len) { > > > > strcpy(buf, tmp); > > > > - return ret; > > > > } > > > > - return 0; /* nothing written or modified */ > > > > + > > > > + free(tmp); > > > > + > > > > + return ret; > > > > } > > > > > > Not sure why it needs a tmp buffer anyway? > > > > The temporary buffer is to ensure that in the case that the data doesn't > > fit in the buffer, the buffer remains unmodified. The reason for this is > > that when building up the json response we always have a valid json string. > > i guessed this but you've now confirmed it. it makes sense in general > that if the callee signals an error to the caller that the caller shall > not observe any side-effects to do so is to take a dependency on what is > more often than not an internal implementation detail. > > > > > For example, suppose we are preparing a response with an array of two > > strings. After the first string has been processed, the output buffer > > contains: '["string1"]'. When json_snprintf is being called to add string2, > > there are a couple of things to note: > > * the text to be inserted will be put not at the end of the string, but > > before the closing "]". > > * the actual text to be inserted will be ',"string2"]', so ensuring that > > the final buffer is valid. > > However, the error case is problematic. While we can catch the case where > > the string to be inserted overflows/has been truncated, doing a regular > > snprintf means that our output buffer could contain invalid json, as our > > end-terminator would have been overwritten, e.g. '["string1","string2' > > To guarantee the output from telemetry is always valid json, even in case > > of truncation, we use a temporary buffer to do the write initially, and if > > it doesn't get truncated, we then copy that to the final buffer. > > > > That's the logic for this temporary buffer. Now, thinking about it > > yesterday evening, there are other ways in which we can do this, which can > > avoid this temporary buffer. > > 1. We can do the initial snprintf to an empty buffer to get the length that > > way. This will still be slower, as it means that we need to do printf > > processing twice rather than using memcpy to copy the result. However, it's > > probably less overhead than malloc and free. > > 2. AFAIK, the normal case for this function being called is with a single > > terminator at the end of the string. We can take advantage of that, by > > checking if the '\0' just one character into the string we are printing, > > and, if so, to store that once character. If we have a snprintf error > > leading to truncation, it then allows us to restore the original string. > > > > My suggestion is to use a combination of these methods. In json_snprintf > > check if the input buffer is empty or has only one character in it, and use > > method #2 if so. If that's not the case, then fallback to method #1 and do > > a double snprintf. > > > > Make sense? Any other suggestions? > > your suggestion seems okay to me, aside from that there's always using > some fixed sized buffer but i'm guessing this being json it's difficult > to choose a reasonable constant size for a stack allocated buffer. > Yes, choosing a reasonable size is very difficult. We could be snprintf-ing a string containing a json-ized object a couple of KB long. I think suggestion #2 above should cover most cases, in which case using your original suggestion of malloc would be ok too for the rare case (if ever) where we don't just have one terminator on the end. /Bruce