From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.windriver.com (mail.windriver.com [147.11.1.11]) by dpdk.org (Postfix) with ESMTP id 3F0E85920 for ; Mon, 1 May 2017 19:30:25 +0200 (CEST) Received: from ALA-HCA.corp.ad.wrs.com (ala-hca.corp.ad.wrs.com [147.11.189.40]) by mail.windriver.com (8.15.2/8.15.1) with ESMTPS id v41HUNwD009455 (version=TLSv1 cipher=AES128-SHA bits=128 verify=FAIL); Mon, 1 May 2017 10:30:23 -0700 (PDT) Received: from yow-masselst-lx1.localnet (128.224.21.51) by ALA-HCA.corp.ad.wrs.com (147.11.189.50) with Microsoft SMTP Server id 14.3.294.0; Mon, 1 May 2017 10:30:22 -0700 From: Mark Asselstine To: Thomas Monjalon CC: alloc , , "Wang, Weiwei" Date: Mon, 1 May 2017 13:30:22 -0400 Message-ID: <1917888.H2NkN7KKrQ@yow-masselst-lx1> Organization: Wind River User-Agent: KMail/5.2.3 (Linux/4.8.0-46-generic; KDE/5.26.0; x86_64; ; ) In-Reply-To: <2030753.g6EQP89X4l@xps> References: <24c64816-8084-b5fd-0b10-3db3143500ec@windriver.com> <4627597.6XQZM4AtIa@yow-masselst-lx1> <2030753.g6EQP89X4l@xps> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Mailman-Approved-At: Mon, 01 May 2017 19:48:41 +0200 Subject: Re: [dpdk-dev] [PATCH] tools/dpdkdevbind.py: remove call to lower case for mod path 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, 01 May 2017 17:30:25 -0000 On Monday, May 1, 2017 6:29:18 PM EDT Thomas Monjalon wrote: > 01/05/2017 17:33, Mark Asselstine: > > On Monday, May 1, 2017 11:24:13 AM EDT Mark Asselstine wrote: > > > On Friday, April 28, 2017 11:38:17 AM EDT Thomas Monjalon wrote: > > > > 25/11/2016 04:16, alloc: > > > > > If the module path has upper case chars, the dpdk-devbind.py script > > > > > will > > > > > crunch them to lower case. This will result in the script never > > > > > finding a module. > > > > > > > > I wonder why this "lower" was done. > > > > I'm afraid we are missing something. > > > > Nobody else is complaining about this issue. > > > > Please confirm it is a real issue. > > > > > > The commit (d6537e6a7432ea9cf39fc4ab2112d4bce0e9fe57) that brought in > > > the > > > lower() call does not document any specific reason for its inclusion. So > > > unfortunalely we can't rely on historic wisdom to rule out this change. > > > > > > We can however look at the source to determine that the lower() call is > > > bogus. > > > > > > --- usertools/dpdk-devbind.py --- > > > > > > # check using depmod > > > > > > try: > > > depmod_out = check_output(["modinfo", "-n", mod], > > > > > > stderr=subprocess.STDOUT).lower() > > > > > > if "error" not in depmod_out: > > Actually, looking at this I can see only one reason for the lower(), and > > that is to catch 'ERROR vs. Error vs. error vs. ...". > > Yes it is exactly what I was thinking. > > > So Alloc can you make an > > > > additionaly change and in the line above change it to: > > if "error" not in depmod_out.lower(): > Good suggestion. > > > That should address any concerns. Of course this still leaves a corner > > case of 'error' being in the path, but the place this would exist would > > be in the kernel version and extra-version and I doubt many folks put > > 'error' in there. > > > > Mark > > > > > path = depmod_out.strip() > > > > > > if exists(path): > > > return path > > > > > > except: # if modinfo can't find module, it fails, so continue > > > > > > pass > > > > > > --- > > > From this we know that depmod_out will have the lowercase version of the > > > path to the module. We also know that exists() is case sensitive and > > > therein lies the issue. Since the path to the module will include kernel > > > attributes the only reason folks may not be seeing this issue as that > > > the > > > attributes are only numbers, periods and lowercase alpha chars. Add a > > > singe > > > upper alpha char in the kernel extended name and users will have this > > > issue, as we have seen it. > > Which kernel module has an upper case character? Not any kernel module, but the kernel version/extra-version can easily have non-lowercase alpha chars. > > > > Can Alloc improve the commit log to make this clear, sure. But the > > > change is good and should be merged. > > It seems Alloc is not his real name? > Please use your real name for SoB (Chunguang Yang?). Seems he is no longer with Wind River so I will take care of sending an updated patch with updated commit log. Mark