From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f172.google.com (mail-wr0-f172.google.com [209.85.128.172]) by dpdk.org (Postfix) with ESMTP id 1B3A69145 for ; Thu, 17 Aug 2017 17:25:57 +0200 (CEST) Received: by mail-wr0-f172.google.com with SMTP id 5so21991523wrz.5 for ; Thu, 17 Aug 2017 08:25:57 -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=S6aoEKV6OyqgpUB7G7DmrwX550yLfrJgt87e9SdlZgQ=; b=xT51QasODn76WctOTOOJxeIi5hj8OyuGTHHcJ910ZoShJ2uIlL/k/axRe7+GaEbwW/ jnTkSprVCLp5RrvRd9pfI6e3AqsuBStKUO/t83GrmkYsyulw+Ins9JqNuWQvqSElPG1t 3LAI8QDxaAdjOTdyJAXKRFAy9+G7Vw7fMnWpg83bSlNdEq1h+lWxHHi5C7GCqU6SqmM9 XZX6wMZMJdjld0FiQJs762DaNe7BxQeOKuNyMs1bxCUEwuE5tg5K2TPvbr108lgTgk7W MVlSSJQLccFz5oLbXZnWXYq+pnR67MsUFYtK9up92Z8M6U0iSD5v4ZfR5lvLSan+sMqu gtSQ== 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=S6aoEKV6OyqgpUB7G7DmrwX550yLfrJgt87e9SdlZgQ=; b=NJsqq72lGoUOZiiN92QewepmLdwp7o4QKKh0vJiUzmctShJ7Z1oT383UGyvaquPVx8 yeshN3yH6NHGXbVRvHyM0DDRoSSG39SVBy23woDrUMhxyJfxJks9l5F3n1Fe0CIsfQN6 UZtqvcdwwwJ6nxadwhSK5s+7KtLUHD6GRwyJ5MSp0OudbAxDZA/Fin09P3rUIQD8dMvm Z6Kt3TiC4BdoIQFaCzqqVxqe7T69Pi0mUQ9cQq7QVenYEtCh+V+5aA/DTzjKVIZ0yzWo y1vLokRhAy7tuK84D8L8FoX1mNL08XN9Oc06g7JyNyb4E70g9LPERVZdpsgmIg9eYP/Q 9jHg== X-Gm-Message-State: AHYfb5juNtlqFFLuPeTLDBS9/FVMVcjm1+OvyyGgP2WidhuEH4efkcox 5LL4EH3r64mQQDMK X-Received: by 10.28.30.77 with SMTP id e74mr1630548wme.172.1502983556517; Thu, 17 Aug 2017 08:25:56 -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 c68sm3886835wmh.21.2017.08.17.08.25.55 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 17 Aug 2017 08:25:55 -0700 (PDT) Date: Thu, 17 Aug 2017 17:25:46 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: dev@dpdk.org, Ori Kam , stable@dpdk.org Message-ID: <20170817152546.GP8124@bidouze.vm.6wind.com> References: <1502979581-27282-1-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1502979581-27282-1-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] net/failsafe: fix parameters parsing X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Aug 2017 15:25:57 -0000 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. > > 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. 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. > 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) 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