From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0065.outbound.protection.outlook.com [104.47.1.65]) by dpdk.org (Postfix) with ESMTP id 2D2691B2BF for ; Tue, 16 Jan 2018 17:17:47 +0100 (CET) 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=8CQBcJOuKPhrpiMnaywa6eSdnga8vXZhGKGjnZDan8w=; b=X0EwMwL35lLMmkS3iHS55kE1NBDIIMoV2fa1XINU14ay22HDDQ6HW37NOALoNbn2Yp+2tcR+b5jUoxoUd7++4Mm4KoAMV3Gm1q9S5H0YSTo9Res6T+PcogQc88H9f4Zz/SvfbyOwcX3kP8qs4v8bPG0MVn/i3mlWEeMVJBRMJqc= Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com (52.133.21.26) by AM6PR0502MB4070.eurprd05.prod.outlook.com (52.133.30.157) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.407.7; Tue, 16 Jan 2018 16:17:45 +0000 Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com ([fe80::6c28:c6b3:de94:a733]) by AM6PR0502MB3797.eurprd05.prod.outlook.com ([fe80::6c28:c6b3:de94:a733%13]) with mapi id 15.20.0407.012; Tue, 16 Jan 2018 16:17:45 +0000 From: Matan Azrad To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: Ferruh Yigit , Thomas Monjalon , "dev@dpdk.org" , "stephen@networkplumber.org" , Adrien Mazarguil Thread-Topic: [PATCH v3 2/8] net/failsafe: add "fd" parameter Thread-Index: AQHTjrh01IHWs9qDAkqp6t/EgOKk1qN2WjUAgABTGtA= Date: Tue, 16 Jan 2018 16:17:45 +0000 Message-ID: References: <20171222173846.20731-1-adrien.mazarguil@6wind.com> <1515509253-17834-1-git-send-email-matan@mellanox.com> <1515509253-17834-3-git-send-email-matan@mellanox.com> <20180116105443.llmifnjgp2ntov7s@bidouze.vm.6wind.com> <20180116111907.a2wi6kwv7g7bql5u@bidouze.vm.6wind.com> In-Reply-To: <20180116111907.a2wi6kwv7g7bql5u@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: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM6PR0502MB4070; 7:Byv0ZymgjGpx/5lfMF3gFXbGYwspI5O1bHNMWNr5rioAbEDIUcJ5S34hQDT3vq+ILRhQ33/dhvltX0YcBguyDhmbkDXWubi0xqVzKlrnDcwYrbb5K8sHv20DFppz6quxZoK9nMMuZEh7kbhGp+5rVXVnidUSIQ0jDIwj2f1J+zfbxvmhvbiw7Sa36HlByd82nZmPn3QZj8R9HIwQvDzkJBm6hqHRfnsXu+dKfc6eLSEez7vdZNyt3ushks50O0Bn x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 7fc9d56c-a011-4b92-9c3f-08d55cfcad96 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(4604075)(3008032)(48565401081)(2017052603307)(7153060)(7193020); SRVR:AM6PR0502MB4070; x-ms-traffictypediagnostic: AM6PR0502MB4070: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(278428928389397)(211171220733660); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040470)(2401047)(8121501046)(5005006)(93006095)(93001095)(3231023)(944501161)(10201501046)(3002001)(6055026)(6041268)(20161123564045)(20161123560045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(6072148)(201708071742011); SRVR:AM6PR0502MB4070; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:AM6PR0502MB4070; x-forefront-prvs: 0554B1F54F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(376002)(346002)(39860400002)(39380400002)(396003)(189003)(24454002)(199004)(8936002)(9686003)(81156014)(3660700001)(33656002)(2950100002)(3280700002)(97736004)(53936002)(6916009)(99286004)(7696005)(316002)(54906003)(86362001)(66066001)(478600001)(59450400001)(102836004)(14454004)(6506007)(6436002)(55016002)(76176011)(8676002)(81166006)(229853002)(4326008)(25786009)(6246003)(68736007)(106356001)(105586002)(2906002)(5660300001)(3846002)(93886005)(2900100001)(74316002)(305945005)(5250100002)(7736002)(6116002)(26005); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR0502MB4070; H:AM6PR0502MB3797.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: h8oYcqWWVIhZiOOG83C61jZoyxm2hliwUcfBehD1xbVt6lFGWgKEl5L3kSS22UWc1csg3uskxgC/xPj1J7wKZQ== 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-Network-Message-Id: 7fc9d56c-a011-4b92-9c3f-08d55cfcad96 X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Jan 2018 16:17:45.5542 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0502MB4070 Subject: Re: [dpdk-dev] [PATCH v3 2/8] net/failsafe: add "fd" parameter 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: Tue, 16 Jan 2018 16:17:47 -0000 Hi Gaetan OK for all, will change it. From: Ga=EBtan Rivet, Tuesday, January 16, 2018 1:19 PM > Hi again, >=20 > made a mistake in reviewing, see below. >=20 > On Tue, Jan 16, 2018 at 11:54:43AM +0100, Ga=EBtan Rivet wrote: > > Hi Matam, Adrien, > > > > On Tue, Jan 09, 2018 at 02:47:27PM +0000, Matan Azrad wrote: > > > From: Adrien Mazarguil > > > > > > This parameter enables applications to provide device definitions > > > through an arbitrary file descriptor number. > > > > Ok on the principle, > > > > > > > > > @@ -161,6 +165,73 @@ typedef int (parse_cb)(struct rte_eth_dev *dev, > > > const char *params, } > > > > > > static int > > > +fs_read_fd(struct sub_device *sdev, char *fd_str) { > > > + FILE *fp =3D NULL; > > > + int fd =3D -1; > > > + /* store possible newline as well */ > > > + char output[DEVARGS_MAXLEN + 1]; > > > + int err =3D -ENODEV; > > > + int ret; > > > > ret is used as flag older, line counter and then error reporting. > > err should be the only variable used for reading errors from function > > and reporting it. > > > > It would be clearer to use descriptive names, such as "oflags" and "nl" > > or "lcount". I don't really care about one additional variable in this > > function, for the sake of expressiveness. > > > > > + > > > + RTE_ASSERT(fd_str !=3D NULL || sdev->fd_str !=3D NULL); > > > + if (sdev->fd_str =3D=3D NULL) { > > > + sdev->fd_str =3D strdup(fd_str); > > > + if (sdev->fd_str =3D=3D NULL) { > > > + ERROR("Command line allocation failed"); > > > + return -ENOMEM; > > > + } > > > + } > > > + errno =3D 0; > > > + fd =3D strtol(fd_str, &fd_str, 0); > > > + if (errno || *fd_str || fd < 0) { > > > + ERROR("Parsing FD number failed"); > > > + goto error; > > > + } > > > + /* Fiddle with copy of file descriptor */ > > > + fd =3D dup(fd); > > > + if (fd =3D=3D -1) > > > + goto error; > > > + ret =3D fcntl(fd, F_GETFL); > > > > oflags =3D fcntl(...); > > > > > + if (ret =3D=3D -1) > > > + goto error; > > > + ret =3D fcntl(fd, F_SETFL, fd | O_NONBLOCK); > > > > err =3D fcntl(fd, F_SETFL, oflags | O_NONBLOCK); Using (fd | O_NONBLOCK= ) > > is probably a mistake. > > >=20 > This is sneaky. err is -ENODEV and would change to -1 on error, losing so= me > meaning. >=20 > > > + if (ret =3D=3D -1) > > > + goto error; > > > + fp =3D fdopen(fd, "r"); > > > + if (!fp) > > > + goto error; > > > + fd =3D -1; > > > + /* Only take the last line into account */ > > > + ret =3D 0; > > > + while (fgets(output, sizeof(output), fp)) > > > + ++ret; > > > > lcount =3D 0; > > while (fgets(output, sizeof(output), fp)) > > ++lcount; > > > > > > > + if (feof(fp)) { > > > + if (!ret) > > > + goto error; > > > + } else if (ferror(fp)) { > > > + if (errno !=3D EAGAIN || !ret) > > > + goto error; > > > + } else if (!ret) { > > > + goto error; > > > + } > > > > These branches seems needlessly complicated: > > > > if (lcount =3D=3D 0) > > goto error; > > else if (ferror(fp) && errno !=3D EAGAIN) > > goto error; > > >=20 > Here err would have been set to 0 previously with the fcntl call, meaning= that > jumping to error would return 0 as well. >=20 > I know Adrien wanted to avoid the usual ugly >=20 > if (error) { > err =3D -ENODEV; > goto error; > } >=20 > But this kind of sneakiness is not easy to parse and maintain. If someone > adds a new path of error later, this kind of subtlety *will* be lost. >=20 > So between ugliness and maintainability, I choose maintainability (being = the > maintainer, of course). >=20 > -- > Ga=EBtan Rivet > 6WIND