From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f169.google.com (mail-wr0-f169.google.com [209.85.128.169]) by dpdk.org (Postfix) with ESMTP id CC69B58CB for ; Thu, 17 Aug 2017 18:25:08 +0200 (CEST) Received: by mail-wr0-f169.google.com with SMTP id y96so45491380wrc.1 for ; Thu, 17 Aug 2017 09:25:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=7qqvH9KJlUXBe6LNLj8JozetKdqTLSnpqlJot9E205g=; b=i2ozNP4PzUdDtpcjscusJC9NhoXfxwo+t4SEHhIhNpR4kCj6WljaU5xN5MmJ333CWT MqvfKl1QcvPkwMriDvzyDRWtVVIsjh9UINnXJqb6OWKZv/vQmQiA8D18sZwGTGhIrH8o 4U+5XgHpgHfxVtJLWXwI9YOzcKnUFMuGGznpVAN406jWNVO2rM3m8nLGSughJIIPsjQo oq7VVAXAnclzb6aWyXlHTu5kgRrdOfUg/GOLLYEeSuoZyDy4Y5FcKMFx+hHYkCkJjpqX SOobBbnc4EBmYac9E0vBeK9wccQKZwvoUBbTIc2LxbkB5uHHYrNZdGKbyGj6HV0B6fQn 7eKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=7qqvH9KJlUXBe6LNLj8JozetKdqTLSnpqlJot9E205g=; b=uiWCI4QPelz3JDLUwJUBZEwxfY+0pdf69uFTxg8DdJMHpsfdFLha1kx3jFmBiTSjKE PR/T4ucLFjAvCF29r1QRMeRtbhhjhgR3SMlxQVLGbeStEEgRjcatbSIQ8/HZdpwhZjfU z8/e2OI5CimLNhXgvTe+5W+S2TD6a969X8qk+FVtTI5Zww4MK25YcXdDP7k8PJhPVEy1 OpyeQhTM8Gn6KupeTpaMbOc1PY7zqoX63sGqdks1dyqxy4MgUPYjRLz25R9pZ+wAB2AP l+ydB7xBn3GFd6DGBCf3W5DRLMJz6lEz+59zNPWpgq846BAmyfKjkQfFxmaVkNc4O0kH zbxg== X-Gm-Message-State: AHYfb5gGLgiQCBsXDDbF/347NrpVqQ0vdY0pVBi+FXCNR+eWx4HtUHiE k9U27RKGPgqqEIGd X-Received: by 10.223.176.35 with SMTP id f32mr4319206wra.198.1502987108243; Thu, 17 Aug 2017 09:25:08 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id w20sm3113534wra.27.2017.08.17.09.25.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 17 Aug 2017 09:25:07 -0700 (PDT) Date: Thu, 17 Aug 2017 18:24:58 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: "dev@dpdk.org" , Ori Kam , "stable@dpdk.org" Message-ID: <20170817162458.GQ8124@bidouze.vm.6wind.com> References: <1502979581-27282-1-git-send-email-matan@mellanox.com> <20170817152546.GP8124@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-stable] [PATCH] net/failsafe: fix parameters parsing X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Aug 2017 16:25:08 -0000 On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote: > > > > -----Original Message----- > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com] > > Sent: Thursday, August 17, 2017 6:26 PM > > To: Matan Azrad > > Cc: dev@dpdk.org; Ori Kam ; stable@dpdk.org > > Subject: Re: [PATCH] net/failsafe: fix parameters parsing > > > > Hi Matan, > > > > Thanks for spotting the bug. > > > > On Thu, Aug 17, 2017 at 05:19:41PM +0300, Matan Azrad wrote: > > > The corrupted code used wrongly snprintf return value as the number of > > > characters actually copied, in spite of the meanning is the number of > > > characters which would be generated for the given input. > > > > > > It caused to remain zerod bytes between the failsafe command line non > > > sub device parameters indicates end of string. > > > > > > Hence, when rte_kvargs_parse tried to parse all parameters, it got end > > > of string after the first one and the others weren't parsed. > > > > > > So, if the mac parameters was the first in command line it was taken > > > while hotplug_poll was left default, and vice versa. > > > > > > The fix updates the buffer index by dedicated variable contains the > > > copy size, by the way uses memcpy instead of snprintf which is good > > > enouth for this copy scenario. > > > > Actually snprintf should still be used. > > > Why? > We just want to copy from buffer to buffer, no needs snprintf overheads > If actually we are not using complicated format. > snprintf plays nice with strings and ensure that it will be terminated by a '\0'. It is generally preferable, particularly considering that the parameter string we are building here is meant to be parsed by another lib (rte_kvargs). > > > > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Matan Azrad > > > --- > > > drivers/net/failsafe/failsafe_args.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/failsafe/failsafe_args.c > > > b/drivers/net/failsafe/failsafe_args.c > > > index 1f22416..0a98b04 100644 > > > --- a/drivers/net/failsafe/failsafe_args.c > > > +++ b/drivers/net/failsafe/failsafe_args.c > > > @@ -271,7 +271,7 @@ static int > > > fs_remove_sub_devices_definition(char params[DEVARGS_MAXLEN]) { > > > char buffer[DEVARGS_MAXLEN] = {0}; > > > - size_t a, b; > > > + size_t a, b, temp; > > > > temp is not descriptive enough. > > What are about a, b, i ? > > > You are declaring this variable here because you want to re-use it instead of > > `start`. This is an overreach however, this fix must be restricted to the actual > > bug. > > temp is helping to address the original bug, don't we want to reuse variable > Instead of 2 if statement variables, maybe other name for all? > The compiler should be able to see that their use does not overlap. The issue is that temp is meant to describe a length limit in one branch and be a marker of a starting point in another. Thus both variable should be named differently to make the intent clear. When declaring temp (or saved_val) in the higher-scope, you are thus forced to use generic, non-descriptive name precisely because you are sharing the variable between two different uses. > Something like: > a=> start > b=> end > i => next > temp=> saved_val > > > > > > > int i; > > > > > > a = 0; > > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char > > params[DEVARGS_MAXLEN]) > > > ERROR("Invalid parameter"); > > > return -EINVAL; > > > } > > > - if (params[b] == ',' || params[b] == '\0') > > > > Declare the temporary variable in this scope, with a descriptive name such as > > "len", "length", "param_len" or something close. > > > > > - i += snprintf(&buffer[i], b - a + 1, "%s", ¶ms[a]); > > > - if (params[b] == '(') { > > > - size_t start = b; > > > + if (params[b] == ',' || params[b] == '\0') { > > > + temp = b - a + 1; > > > > The value here should be "b - a". > > If a != 0 however, then we are parsing a new parameter and buffer already > > contains at least one. A comma should be added to separate the two. > > > > An example might clarify what I mean: > > > > if (params[b] == ',' || params[b] == '\0') { > > size_t param_len = b - a; > > > > if (a) > > param_len += 1; > > snprintf(&buffer[i], param_len + 1, "%s%s", > > a ? "," : "", ¶ms[a]); > > i += param_len; > > } > > > > The conditionals about `a` are ugly however, if you find a better way to write > > those you are most welcome :). > > > > > + memcpy(&buffer[i], ¶ms[a], temp); > > > + i += temp; > > > + } else if (params[b] == '(') { > > > + temp = b; > > > b += closing_paren(¶ms[b]); > > > - if (b == start) > > > > I think the last comma is harmless for next parse > But I can just change the last coma to '\0' in the end of function(if exists). > But, This solves another issue, don't it? Maybe in different patch? > It might be harmless, but I prefer having a clean output. Which other issue? Handling the commas seems in line with properly copying regular parameters. You could certainly check for the presence of a comma, but you already see how ugly the code flow would be ("if it exists"), and you will keep a dirty string between two loops, with possibly other branches wanting to return on error. I prefer having a compact, self-contained logic addressing kvargs which should not leak conceptual gotcha that the next programmer would need to be wary of when trying to touch the code in other parts of the function. > > The changes should be limited to the actual bug. No need to change this. > > > > > + if (b == temp) > > > return -EINVAL; > > > b += 1; > > > if (params[b] == '\0') > > > -- > > > 2.7.4 > > > > > > > Thanks again for the debug and sorry for being nitpicky. > > > > -- > > Gaëtan Rivet > > 6WIND > > Regards, > Matan Azrad. -- Gaëtan Rivet 6WIND