Re: [dpdk-dev] [PATCH] tools/dpdkdevbind.py: remove call to lower case for mod path

[Mark Asselstine <mark.asselstine@windriver.com>]

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