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 98D63428C6; Tue, 4 Apr 2023 19:25:57 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 27A6240EE3; Tue, 4 Apr 2023 19:25:57 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 3091440A7E for ; Tue, 4 Apr 2023 19:25:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1680629155; x=1712165155; h=date:from:to:cc:subject:message-id:references: in-reply-to:mime-version; bh=M7ObP3NYBzm6y4g6CzYpHbd2NpAp9wGFfcJ/JEQXfpw=; b=SQ17+BbBzC30JRkpZeQqBPNhw0t/urLpTLPsVJc3fpum6X1fF7Xr8ZZy 2fxgW9nEs51rEwhEZM8YkiGXH4sg+lFSsZPTneANJV8ZTPZo8KSYkR0v/ SvYVA9l//MsOiQx3HhA61Zm1JYBQM72kzTzibX8ADYC4mxTrTq54ns87b GCxx9MvAjVBdJ0/sve5Ge0MQg8aFmeArdoBd9JjF2SQ20IOKyGTqDN1C6 cTRA5B7qeDkvOUYS3+Ozy8q6nqz5r7eGkG959jECtW5IotmColK8C4rap rzbO5SBhBsTYBfBzmMOztg1F9v5tAyIgCh8+fq9NlzSQJ4jJoQsKghsPs Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10670"; a="343960844" X-IronPort-AV: E=Sophos;i="5.98,318,1673942400"; d="scan'208";a="343960844" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Apr 2023 10:25:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10670"; a="688976699" X-IronPort-AV: E=Sophos;i="5.98,318,1673942400"; d="scan'208";a="688976699" Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmsmga007.fm.intel.com with ESMTP; 04 Apr 2023 10:25:53 -0700 Received: from orsmsx611.amr.corp.intel.com (10.22.229.24) 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.21; Tue, 4 Apr 2023 10:25:52 -0700 Received: from orsmsx602.amr.corp.intel.com (10.22.229.15) by ORSMSX611.amr.corp.intel.com (10.22.229.24) 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 10:25:52 -0700 Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) 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 via Frontend Transport; Tue, 4 Apr 2023 10:25:52 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.177) by edgegateway.intel.com (134.134.137.103) 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 10:25:51 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TSvtVGcnsmrLKreteNUuCExPfvlYuxOq+F8/mNG8apBDHQdt3InonywWtU3pRLR73zunbn6hcKaq5w2vJ/ppcleCakNhx8nr4n8kYwK4ohb+p1T/jToI97b0G/Ehjz90goRDybiidYvg97M481HzCGEO/B0jwkC6vZLhakyydS2gcJpdEaUEgGzrxi0ZxjUmZ/nEkUWhtpMvGiHMHMeIBEmAgm8ZzqFZc9XGScgLr6nzhWFDVIP/kjZm0youQtzY6IXdugA+fXYKr9zL2AyvrTdkE7FweGH4fysIUCeSRB7a9/xRZ96Z1qxIyJv76vKrpuIvNjy95udYxzaw96/qeg== 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=hk0xmOVvVvhnhnXh5Lk4BxJSfWzBdsaxIPI5uNZpGsE=; b=Qmkm85pQeifnvkm6SdejxERhxVueaQOUMIoM1h7+7/xbc0Xd+W/HYI/UXs2l/FZHnAXblNlOptKG0dInQ1Eu1JYAcJaE22KB1ob7GrfU6Ow4ytJ2eQsRQj/9My4+X+V8lY116/PmKEDSj8Fz/wztsYaa9G7mPqs0biBYacTsc02hII/hwnPD2MmQ5+uOXjHNmRKbK0pHarbAdDhMkouor6Y+G3weuZdwIYr3nalAvjySijIImhnezOHhxC+dAO/BT7A2BVN2f/Pi3WkP48/hEYIU+RzGT+rWXO1jJa9I26qM5marwuwyT1AhorPSPuLC+a+MXoLZm2AZ+Icw1qAu8g== 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 BY1PR11MB8032.namprd11.prod.outlook.com (2603:10b6:a03:524::8) 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 17:25:50 +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 17:25:50 +0000 Date: Tue, 4 Apr 2023 18:25:42 +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> <20230404164446.GF18560@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20230404164446.GF18560@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-ClientProxiedBy: LO2P123CA0037.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600::25) To DS0PR11MB7309.namprd11.prod.outlook.com (2603:10b6:8:13e::17) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DS0PR11MB7309:EE_|BY1PR11MB8032:EE_ X-MS-Office365-Filtering-Correlation-Id: 3c935816-de05-46d4-b0ae-08db3531a262 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 0kTM2F8b4JHCSRFD6Hdqm3a7ByXA6+MgLE7u2eBkjQE6xSEu8DyKFSFnZw9l508Mjbfm6hqNBLjbrOUZphJeEpWm0xUVPrKfI1UppoWh8OjPWkyqZ4ycZij7F58J0+HY6ups3rQ7bLYsMzjms9gxuGGBodNcH0CSRkeHZXgx0SHj+5MX0pjG+mZ02cU2L7S4y8l5364JI565IRzsjasQrYgOK5IYpuqMGe4gcQfiErmVlQDzo1nHvW3C/gF9wd0FOqEvoCxmDesAZTAOJnHejeTed4S1g1c5Vs7g6frqN4JhUJe2bZAmpqfmA5xzk+qirM3PWo+rYTaSRwpmUmhRpobbYIzCPQORmbn1bNKG3JoXYZ45hcSw6aUeW+7sYsbYQrm+eUXAaFrLcTGX/laYI5EXP1lIh0X6cRv5530ExLUdWvYVh5sH8jl4uCirPiRdeeTWLS6HFct2fJc7mNjabePmB8RfcC79EscgRs6PSFSrOOGqWWxf8/J/soefaopZWs2Ld8WiDwUsxFTsV9dKVMCYuHtyZAbhK+fo7Kayuy6COOGuLk1MruFv2z30qO/f 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)(39860400002)(396003)(366004)(376002)(136003)(346002)(451199021)(83380400001)(26005)(186003)(86362001)(66556008)(4326008)(6916009)(66946007)(8676002)(82960400001)(38100700002)(8936002)(66476007)(2906002)(44832011)(41300700001)(478600001)(316002)(6506007)(6512007)(5660300002)(6666004)(6486002); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?yFqIi2+NrGvWhayq5Qr3sJ0BMQYzqzxljfwkRMHgrHBUQye2BC0Qv6ohUAq8?= =?us-ascii?Q?jEfn1IVTN++yZjIRtw1Z0Sq+0nuUMMDQxa9GSQI7sxUj+IgkjoDK5Jf3ohDq?= =?us-ascii?Q?B0x/t8TuA96sYF7XSDYBIHvE0a+vP8A8tMd833STUZs32TMxd56DOsUYKouU?= =?us-ascii?Q?udhdYXyBSsi0GJQPXB7klXYzYVnzQSSH639Um7ih9xsuSW2FJmtY3IulI56Q?= =?us-ascii?Q?Jt9y5Qm/hrzs+4LZVQDqvk3J0FXjJpXDlOeItwGDiYMLp2yq0zEdrOogq0pb?= =?us-ascii?Q?tjztyLMHFS5jhwkXUC8Xtot6/lfu3tHLK6bTMyMCM10N/PT/i7JwN5xLlkcn?= =?us-ascii?Q?7QzYN5LAQ5A4W3t4YJwxMsmKym03oIqAEiurotNNRiSpFISMXMyQ/b4PxdUt?= =?us-ascii?Q?VkQYoJHAuBp4QdBujUopaAH15QIWrxAg2U3JJznmIDCkNCJwLM24+48GlcRJ?= =?us-ascii?Q?jWUGghQ+FdJ6Yi99jXiKJIk8cEBrKiWWlWmBeaQz8EACWrpVjyWVEocf9QC4?= =?us-ascii?Q?1aJK4FMgMhpwYt+2pptzfrgaIEcPs3CzA3MCGont7bsQuG9bpDuOurReotMw?= =?us-ascii?Q?LxOrM5MVD4mvMugNFsQrntP1sdTxN+cZi8qPsCjskoGSxQOl+TStC9MW3Egj?= =?us-ascii?Q?/Pdkax3Xynp3/gw84Cf2ivVhfd5/BajseJ6OgJrXJC4Rw0dLkq7fIcs67aPy?= =?us-ascii?Q?bvqS7wMbrc9yZ9VUjK20d1xszkJ3zkgkagaUlaB8DKdGiV0FYp9H/8jjNYEF?= =?us-ascii?Q?+8zumbV7UAe/WT/YtUwrzYNvNUX9FV2PtEVc7D/VJ1AjNHcRpFpnQcyWMGpy?= =?us-ascii?Q?B6liolSJ4/rgqCDK6umMhzCPWvo9BEElS0N0ResBjP1zd1+3y4ttRxlc2570?= =?us-ascii?Q?mk9N8hV+3Xv36f2SV/2k40LibmOYOOIGHoOA22evqhXsrmUyG2R1GEuiaV4Y?= =?us-ascii?Q?AoM8Jr5f2yUTfDAk89EdTR1Fpcsw6Av8LkevgnoL/sGS1a5vso6KjzeFp70H?= =?us-ascii?Q?TtKycbDrAfkjXJ6g7WrZ2M37FPRxjXLBdLommdu2WikyVL9TFSmJlIX+F8np?= =?us-ascii?Q?46t8czzlGipI7ajJ/qR8vIUbNZAwh4JHWKM1nt6py/U/vK2PBC/nmJyN9Y7M?= =?us-ascii?Q?AMujPkvRrN1J8K+FUyljJinXUA/yl+e9uQ2fGVkdU+tl4exEF2UEamK+vcAa?= =?us-ascii?Q?qdPYyi173rOCnZtOIhwQ8JlHz/HnhTZ/ncFtBY4dpJkaNRE6NlDwSMTToTFM?= =?us-ascii?Q?E10BcXEFh0Uk/UNqntvpYi8eC9/qfLOum6Z7qW1tocpjbKlElHGZvbttdOlT?= =?us-ascii?Q?BwFbHoh8w9rpTUM52bItowlf3ZIjqxCPp/eKFxldHgVFF6dL/hggnEIEFetM?= =?us-ascii?Q?N/5FnsphmwTTew2Lri5R05JRzECDglhKsCMyhUYGgmoxGi50KXp8n0A1SzPZ?= =?us-ascii?Q?d31UH4ELy0dbwCfxrA4hsxDzH1IPEugV5aQ+SXXFQm6/gMDQkJkUvi/tZjaS?= =?us-ascii?Q?TCZR7u6h/HGb5k0dC2z8UxgLEcnBlZg8qyiusdZSEtD3EOU9qMiDolCv8uDt?= =?us-ascii?Q?GxjucuVucQ2HYw/PBDjnXky7DYtCV5JKpYL5MaO0jhamStCxsQcUpgR6lyV8?= =?us-ascii?Q?1Q=3D=3D?= X-MS-Exchange-CrossTenant-Network-Message-Id: 3c935816-de05-46d4-b0ae-08db3531a262 X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7309.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2023 17:25:49.9574 (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: KlScM/6n6qOsniDonIqgdf+vtvGDGcwGzzJi44VlP6FpMdGTIF3HKOpYq12Pq92nmJNiSHvNTHgKQ0QwhMldL/CSitKlFPF1lWPyWkOSW78= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR11MB8032 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:44:46AM -0700, Tyler Retzlaff wrote: > On Tue, Apr 04, 2023 at 05:28:29PM +0100, Bruce Richardson wrote: > > 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. > > haven't checked recently, but i wonder what our normal usermode stack > frame size limit is, which is why alloca() would be scary. > > > > > 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. > > maybe a dumb'd down compromise is to have a fixed stack limit and then > if it is exceeded always just go to malloc/free? > Perhaps. If you like, I have have a try at implementing my own suggestions above tomorrow. I'd like if we can get the "single-character-saving" option working, because that would be the most efficient method of all. /Bruce