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

[Thomas Monjalon <thomas@monjalon.net>]

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?).