[dpdk-dev] [PATCH] tools/dpdkdevbind.py: remove call to lower case for mod path
Thomas Monjalon
thomas at monjalon.net
Mon May 1 18:29:18 CEST 2017
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?
> > 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?).
More information about the dev
mailing list