From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0056.outbound.protection.outlook.com [104.47.2.56]) by dpdk.org (Postfix) with ESMTP id 53641913B; Thu, 17 Aug 2017 20:52:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=2iPVJgNwb9EOAmvl+9mAvOrbyv4kxvpYYJwRcjMT+Bw=; b=L85gEk2i8qyr2bZ3uqzYwkBP51m5xyqdCoEw5uLzk1vPSCooxwGssJOvxzfqA1MQpp2Seggm2L4+lxohaQcn8Ul1WMV/0hRkLfEfIPhHmg2s5vm7MdPSlaUJQwpQczP8DUqyzbUUjn+sDhqrq/OvYVgn1tUPc0yB3GP4C73IssE= Received: from DB6PR0502MB3048.eurprd05.prod.outlook.com (10.172.250.136) by VI1PR05MB3214.eurprd05.prod.outlook.com (10.170.237.159) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1341.21; Thu, 17 Aug 2017 18:52:48 +0000 Received: from DB6PR0502MB3048.eurprd05.prod.outlook.com ([fe80::e938:efe9:effc:7ef8]) by DB6PR0502MB3048.eurprd05.prod.outlook.com ([fe80::e938:efe9:effc:7ef8%14]) with mapi id 15.01.1341.024; Thu, 17 Aug 2017 18:52:48 +0000 From: Matan Azrad To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , Ori Kam , "stable@dpdk.org" Thread-Topic: [PATCH] net/failsafe: fix parameters parsing Thread-Index: AQHTF20gviU1FquxMkGyQ9F2MipUdaKIrHzQgAAPZQCAABa8wA== Date: Thu, 17 Aug 2017 18:52:48 +0000 Message-ID: References: <1502979581-27282-1-git-send-email-matan@mellanox.com> <20170817152546.GP8124@bidouze.vm.6wind.com> <20170817162458.GQ8124@bidouze.vm.6wind.com> In-Reply-To: <20170817162458.GQ8124@bidouze.vm.6wind.com> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [85.64.136.190] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; VI1PR05MB3214; 6:RSHyaGeuvo7EPzYMBvP9EPukY2DJ0PRq8/ksGZZHaSG+WrvoPdZuksOVpiDz3qTakIQIghf3bGYIOBd6qbdWQUwZEIi7oWyXqbMSzIBU70fRM1CmUWFRo4yHT9wUUt0/Xj5uB9M/qIna2gIsMjO6aGepn8ZfOPzuOL56LygqpD1J2JKj/3tl0axlx3RNdjZEyjt9BmTdg31Rck+Sdo2wqAE+rxl0YQ9uHP6c1vQMEqxqGojDxyaP0HERxWvLYwv2nfsgn4hRck3ePp0Ra/aDNlxOXsu14ezBrlNu2Gjh3MrnrdS1B6MQpQeot7K8W6NLt5fYIPYH9fcuR5oxfVNirA==; 5:0lJRdNq2j4zW6QkiI6Pe2dLUQ74b2pcjdiOTHoYQInNOtTQDuApJkwJJ3aU3GjdfXMNyobyzt/Hn6vJdaMKm7/FWVEEvUqly5rzNytF4ZZTFMsjdFAbcPp3p8MTyx/ZgJsyh2rzPg2xCOkLFah5/Sg==; 24:1NXn5juTuYWVkFvMGudt2BUE/dTTo+ucv2BM6tZEiCJHJwoa2G2917IPWE6EwiaaehcyMyChO3M/lwmlTD9DUZB2+jntBBeVCt+DKNrVweE=; 7:zaUa9MGbWKaE/7RegQpTaiZwuBZnJpfVs4UaabMkm58hlVdVizNniJGUsahz/bu0MzbFZSS8CyUPKFOH4d0uGXIx58EjQUaAPGmoTPOV4ye9NmuiWgXOiLw7sfXyMjoIHtg1PXd88g68AVtKCu9t0+12be6y4mve0/qZMM55EQRPc4eADk8UOLBTqC9CvRMc1Rsf3JRrRIHgHJwvoGNp8L6jf3RnGpegh4pT3mpJkQQ= x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 1561eb63-30ad-4dd9-05c3-08d4e5a127b6 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(300000503095)(300135400095)(48565401081)(2017052603157)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:VI1PR05MB3214; x-ms-traffictypediagnostic: VI1PR05MB3214: x-exchange-antispam-report-test: UriScan:; x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(8121501046)(5005006)(93006095)(93001095)(10201501046)(3002001)(100000703101)(100105400095)(6055026)(6041248)(20161123564025)(20161123560025)(20161123558100)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:VI1PR05MB3214; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:VI1PR05MB3214; x-forefront-prvs: 0402872DA1 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(39860400002)(199003)(189002)(377454003)(13464003)(43784003)(24454002)(51914003)(3280700002)(3660700001)(68736007)(189998001)(110136004)(7736002)(53546010)(97736004)(25786009)(2906002)(50986999)(81166006)(105586002)(106356001)(101416001)(93886005)(9686003)(8676002)(6246003)(53936002)(305945005)(54906002)(81156014)(54356999)(14454004)(76176999)(478600001)(5250100002)(7696004)(66066001)(8936002)(55016002)(4326008)(6116002)(99286003)(102836003)(3846002)(86362001)(2900100001)(33656002)(5660300001)(6506006)(74316002)(6436002)(2950100002)(229853002)(6916009); DIR:OUT; SFP:1101; SCL:1; SRVR:VI1PR05MB3214; H:DB6PR0502MB3048.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Aug 2017 18:52:48.4037 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR05MB3214 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 18:52:51 -0000 Hi Gaetan > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Thursday, August 17, 2017 7:25 PM > To: Matan Azrad > Cc: dev@dpdk.org; Ori Kam ; stable@dpdk.org > Subject: Re: [PATCH] net/failsafe: fix parameters parsing >=20 > On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote: > > > > > > > -----Original Message----- > > > From: Ga=EBtan 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. > > >=20 > snprintf plays nice with strings and ensure that it will be terminated by= a '\0'. > It is generally preferable, particularly considering that the parameter s= tring > we are building here is meant to be parsed by another lib (rte_kvargs). > In this specific scenario it is not needed because we for sure know that th= e length doesn't pass after '\0'. By using memcpy we can prevent format parsing and other checks done by snpr= intf. > > > > > > > > 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] =3D {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= ? > > >=20 > The compiler should be able to see that their use does not overlap. >=20 This is new for me, thanks for the lesson. I will update it to len & start as you suggested. > 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. >=20 > When declaring temp (or saved_val) in the higher-scope, you are thus forc= ed > to use generic, non-descriptive name precisely because you are sharing th= e > variable between two different uses. >=20 > > Something like: > > a=3D> start > > b=3D> end > > i =3D> next > > temp=3D> saved_val > > > > You didn't answer what are about my suggestions for new names instead of=20 a,b and I which are not descriptive too (like temp :)) > > > > > > > int i; > > > > > > > > a =3D 0; > > > > @@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char > > > params[DEVARGS_MAXLEN]) > > > > ERROR("Invalid parameter"); > > > > return -EINVAL; > > > > } > > > > - if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') > > > > > > Declare the temporary variable in this scope, with a descriptive > > > name such as "len", "length", "param_len" or something close. > > > > > > > - i +=3D snprintf(&buffer[i], b - a + 1, "%s", ¶ms[a]); > > > > - if (params[b] =3D=3D '(') { > > > > - size_t start =3D b; > > > > + if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') { > > > > + temp =3D b - a + 1; > > > > > > The value here should be "b - a". > > > If a !=3D 0 however, then we are parsing a new parameter and buffer > > > already contains at least one. A comma should be added to separate th= e > two. > > > > > > An example might clarify what I mean: > > > > > > if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') { > > > size_t param_len =3D b - a; > > > > > > if (a) > > > param_len +=3D 1; > > > snprintf(&buffer[i], param_len + 1, "%s%s", > > > a ? "," : "", ¶ms[a]); > > > i +=3D 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 +=3D temp; > > > > + } else if (params[b] =3D=3D '(') { > > > > + temp =3D b; > > > > b +=3D closing_paren(¶ms[b]); > > > > - if (b =3D=3D 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? > > >=20 > It might be harmless, but I prefer having a clean output. > Which other issue? Handling the commas seems in line with properly copyin= g > regular parameters. Yes, but it is another issue in this specific copy (don't refer to snprintf= return value)=20 But, it's OK, I think them both can be handle by this patch :) >=20 > You could certainly check for the presence of a comma, but you already se= e > how ugly the code flow would be ("if it exists"), and you will keep a dir= ty > string between two loops, with possibly other branches wanting to return = on > error. >=20 > 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. > If we leave always b-a+1 we potentially can get the next buffer strings: p1,p2...pN, or=20 p1,p2,pN(\0) while i is the index of the character after a comma or '\0' so, for prevent the last comma we can override always in the end of functio= n(not between 2 loops) '\0' like this: if (i >0 ) /* For prevent last comma. */ buffer[i-1] =3D '\0'; instead of using 2 conditions per non sub device param - 'if(a)' and 'a ? "= ," : ""' I think each future programmer can easily understand it. =20 > > > The changes should be limited to the actual bug. No need to change th= is. > > > > > > > + if (b =3D=3D temp) > > > > return -EINVAL; > > > > b +=3D 1; > > > > if (params[b] =3D=3D '\0') > > > > -- > > > > 2.7.4 > > > > > > > > > > Thanks again for the debug and sorry for being nitpicky. > > > > > > -- > > > Ga=EBtan Rivet > > > 6WIND > > > > Regards, > > Matan Azrad. >=20 > -- > Ga=EBtan Rivet > 6WIND Regards Matan Azrad.