From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0062.outbound.protection.outlook.com [104.47.0.62]) by dpdk.org (Postfix) with ESMTP id BF4F97CF8; Mon, 21 Aug 2017 14:02:46 +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=4IzBedAhszmXMtnwd8wYm58+YVIc9ZMae/Ux+SXK+9M=; b=kuxc5HstmdqPcqkHDWd+x5Ba42te5xcTzImYxEZNSFTCsKqpToq+kWKy1LU4PXLXZ75HoLtKzGHmoXSH8ezprtHoaf9BWYc64kAcmK1hGWPmgw3cvXTZYeAL8pzoKppf+3NvbVbsQ0oUx/ZgZOQQomzFwr7q+8il0RVMX3Hm+68= Received: from DB6PR0502MB3048.eurprd05.prod.outlook.com (10.172.250.136) by DB6PR0502MB3016.eurprd05.prod.outlook.com (10.172.247.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1362.18; Mon, 21 Aug 2017 12:02:44 +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.1362.019; Mon, 21 Aug 2017 12:02:44 +0000 From: Matan Azrad To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , "stable@dpdk.org" Thread-Topic: [PATCH v2] net/failsafe: fix parameters parsing Thread-Index: AQHTGmC20+gWTG1XnEWLbnVQh/42yqKOr72Q Date: Mon, 21 Aug 2017 12:02:44 +0000 Message-ID: References: <1503180443-9687-1-git-send-email-matan@mellanox.com> <20170821093426.GV8124@bidouze.vm.6wind.com> In-Reply-To: <20170821093426.GV8124@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; DB6PR0502MB3016; 6:bHEhFmIY0pMsw3jXEvZ5B8t0ibOkeaQA4PZ75KSKHTpyxJzRbonWP4ClkOfolNTuqQTY2PCotEaz6VhLcKh4AA3XxLs3cnhFECtaiW2bpeYHy7wfkN3jSlQ/iQtS5LpjIOvriqeha1q3N87G4rAHbYaEj9+rJ5oHoLahxrLghcNytjXmpBJPmWy+W4wf/aM7hr0KcsS6V81I3pyF+bz42lfJH7EtXNrmtma7FvhWtH35xjoLJsxhT6k3DgSPo3NnBLZYEyCC1cA3O+UDYKoszkmcw3AqVRRE5/0VIEpBeg9MRJzdCoalmVw+1WUmxLz4HffL7C/TBsBp6wxE2X6psw==; 5:QYUA79TDj2B/9yYnLdX6EFGJqHPhy+kFfHednYbmHG088UnhjiQYMZqVGZrWD9+XroosTKd2ofihWvxfCcIAiH3LSBiAIGzZzox+u/dtBK6r21PmR2bOYpH4PumowvOZGk7FG2vXff48BuNrBA5ZZA==; 24:IfRzBDvsugsPTWoRD8RQ3juis1yQFQFCDq1DVJmeiUUre1HhxYQySmBLqtjy79HY/j9V5oGZGrzgq89aWLBu95QNMAcd34/nC5kqNCGE+g4=; 7:xI2tI5Y5IIlipmE47ImmdHv1NW5LPUaK2dCvfLbQPBhnzDKhB4G5Jc6URpYPJdaGEKj4pIMRhnvH3otE+fuxNjggi/xjfEaboXl4HJ9GwffH/8v/kyV67AKqRc5vR+nLWWZ7G8mSO28tGvxCD/7wTNzOw3JjGV3lEvs9n8aUpLkf9EOmYfB2uDI+EF6Nhg+7354UcLqT4FEBbxKqVge4D9JjZ1APhBm5RCxkTcJ3IAA= x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 0361f4f0-e20f-43b3-e9fd-08d4e88c8820 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(48565401081)(300000503095)(300135400095)(2017052603031)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:DB6PR0502MB3016; x-ms-traffictypediagnostic: DB6PR0502MB3016: x-exchange-antispam-report-test: UriScan:(788757137089); 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)(3002001)(100000703101)(100105400095)(93006095)(93001095)(10201501046)(6055026)(6041248)(20161123555025)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123564025)(20161123558100)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:DB6PR0502MB3016; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:DB6PR0502MB3016; x-forefront-prvs: 040655413E x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(39860400002)(199003)(377454003)(189002)(13464003)(24454002)(189998001)(6506006)(5250100002)(3846002)(102836003)(6436002)(6116002)(66066001)(97736004)(4326008)(7696004)(5660300001)(2906002)(101416001)(33656002)(229853002)(2950100002)(74316002)(105586002)(68736007)(106356001)(6916009)(54356999)(50986999)(81166006)(81156014)(8676002)(53546010)(76176999)(478600001)(54906002)(99286003)(86362001)(55016002)(9686003)(53936002)(8936002)(305945005)(7736002)(6246003)(3660700001)(2900100001)(14454004)(3280700002)(110136004)(25786009); DIR:OUT; SFP:1101; SCL:1; SRVR:DB6PR0502MB3016; 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: 21 Aug 2017 12:02:44.2381 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0502MB3016 Subject: Re: [dpdk-dev] [PATCH v2] 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: Mon, 21 Aug 2017 12:02:47 -0000 Hi Gaetan > -----Original Message----- > From: Ga=EBtan Rivet [mailto:gaetan.rivet@6wind.com] > Sent: Monday, August 21, 2017 12:34 PM > To: Matan Azrad > Cc: dev@dpdk.org; stable@dpdk.org > Subject: Re: [PATCH v2] net/failsafe: fix parameters parsing >=20 > Hi Matan, >=20 > On Sun, Aug 20, 2017 at 01:07:23AM +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 validates the last comma removing. > > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") > > Cc: stable@dpdk.org > > > > Signed-off-by: Matan Azrad > > --- > > drivers/net/failsafe/failsafe_args.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/failsafe/failsafe_args.c > > b/drivers/net/failsafe/failsafe_args.c > > index 1f22416..2a5760a 100644 > > --- a/drivers/net/failsafe/failsafe_args.c > > +++ b/drivers/net/failsafe/failsafe_args.c > > @@ -286,10 +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') > > - i +=3D snprintf(&buffer[i], b - a + 1, "%s", ¶ms[a]); > > - if (params[b] =3D=3D '(') { > > + if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') { > > + size_t len =3D b - a + 1; > > + > > + snprintf(&buffer[i], len, "%s", ¶ms[a]); >=20 > There it should be: >=20 > snprintf(&buffer[i], len + 1, "%s", ¶ms[a]); >=20 You right - will be fixed in V3. > This is due to the use of snprintf intead of memcpy. It illustrates actua= lly why > the overhead of using snprintf is worth it, as it would exactly be the si= tuation > where memcpy would copy the last character as a comma and rely on buffer > being clean (well, it is, but that's beside the point :). >=20 I don't think so. We know where is the end of string in this branch - so don't need snprintf = for safety. Actually, it illustrates why in this case we should use memcpy :) =20 > If for any reason (performance? Lunacy?) someone wanted to change the > initialization of buffer (for example, directly working on params instead= of > copying in a temporary buffer first, or simply removing the "=3D {0};" ab= ove > because he is a maniac), then this chunk of code would become unsafe > without snprintf. >=20 For performance\latency I think this one will replace snprintf too. Even for "=3D{0}" removing the branch in the end of function I added(i>0) c= overs it. > > + i +=3D len; > > + } else if (params[b] =3D=3D '(') { > > size_t start =3D b; > > + > > b +=3D closing_paren(¶ms[b]); > > if (b =3D=3D start) > > return -EINVAL; > > @@ -300,6 +304,9 @@ fs_remove_sub_devices_definition(char > params[DEVARGS_MAXLEN]) > > a =3D b + 1; > > } > > out: > > + if (i > 0) > > + /* For last comma preventing. */ > > + buffer[i - 1] =3D '\0'; >=20 > I don't think there is a simple way to keep this in the kvarg branch (tha= t > would involve precomputing the final length of i and checking that we > reached it within said branch -- pretty ugly). >=20 > You would have had to send a v3 anyway due to the OB1 error above. >=20 > Compare with this version: >=20 >=20 > if (params[b] =3D=3D ',' || params[b] =3D=3D '\0') { > size_t param_len =3D b - a; >=20 > if (i > 0) > param_len +=3D 1; > snprintf(&buffer[i], param_len + 1, "%s%s", > i ? "," : "", ¶ms[a]); > i +=3D param_len; > } >=20 > The only added logic is the `a ? "," : ""` conditional, and it allows to = keep all > changes within the kvarg branch, avoiding scattering output string format= ting > logic. >=20 > Please use this instead in your v3. But you use these 2 branches per parameter,=20 I suggest only one branch for any parameters number. Take example of 2 non sub device parameters: You suggest 4 branches and I suggest only 1 to handle the last comma removi= ng. Is it OK for you to use it as is? >=20 > > snprintf(params, DEVARGS_MAXLEN, "%s", buffer); > > return 0; > > } > > -- > > 2.7.4 > > >=20 > -- > Ga=EBtan Rivet > 6WIND