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

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

[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.

Port patch to dpdk-1.7.0

dpdk_nic_bind.py has been renamed to dpdk-devbind.py in 16.11, so
just change file name.
Signed-off-by: Paul Barrette <paul.barrette@windriver.com>
Signed-off-by: Pengyu Ma <pengyu.ma@windriver.com>
Signed-off-by: chunguang yang <chunguang.yang@windriver.com>
---
  tools/dpdk-devbind.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/dpdk-devbind.py b/tools/dpdk-devbind.py
index f1d374d..d66d68c 100755
--- a/tools/dpdk-devbind.py
+++ b/tools/dpdk-devbind.py
@@ -141,7 +141,7 @@ def find_module(mod):
      # check using depmod
      try:
          depmod_out = check_output(["modinfo", "-n", mod],
-                                  stderr=subprocess.STDOUT).lower()
+                                  stderr=subprocess.STDOUT)
          if "error" not in depmod_out:
              path = depmod_out.strip()
              if exists(path):
-- 
2.9.3

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

[Thomas Monjalon]

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.

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

[Mark Asselstine]

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:
            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

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

[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. ...". 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

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

[Thomas Monjalon]

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

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

[Mark Asselstine]

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

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

[alloc]