Re: [PATCH] tools/dpdkdevbind.py: remove call to lower case for mod path
From: Mark Asselstine <hidden>
Date: 2017-05-01 15:33:51
On Monday, May 1, 2017 11:24:13 AM EDT Mark Asselstine wrote:
quoted hunk ↗ jump to hunk
On Friday, April 28, 2017 11:38:17 AM EDT Thomas Monjalon wrote:quoted
25/11/2016 04:16, alloc:quoted
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. ...". So Alloc can you make an
additionaly change and in the line above change it to:
if "error" not in depmod_out.lower():
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.
Can Alloc improve the commit log to make this clear, sure. But the change is
good and should be merged.
Mark