[dpdk-dev] [PATCH] tools/dpdkdevbind.py: remove call to lower case for mod path
Mark Asselstine
mark.asselstine at windriver.com
Mon May 1 19:30:22 CEST 2017
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
More information about the dev
mailing list