[dpdk-dev] [PATCHv8 5/6] pmdinfo.py: Add tool to query binaries for hw and other support information

[Neil Horman]

On Wed, Jun 29, 2016 at 04:12:21PM +0100, Remy Horton wrote:
> 'noon,
> 
> The tool does not work for static PMD libraries (e.g. librte_pmd_i40e.a) -
> is this an intended limitation?
> 
Yes, because .a files are archives, not ELF files.  If you want to run pmdinfo
on an archive, you have to unarchive it first (ar x).  It would be the same if
you had bound them together with tar or zip.

> DPDK doesn't to my knowledge have any coding guidelines for Python, so the
> comments below should be considered advisory rather than merge-blocking
> issues.
> 
> 
> On 17/06/2016 19:46, Neil Horman wrote:
> [..]
> > +++ b/tools/pmdinfo.py
> > @@ -0,0 +1,629 @@
> > +#!/usr/bin/python
> > +# -------------------------------------------------------------------------
> > +# scripts/pmdinfo.py
> > +#
> > +# Utility to dump PMD_INFO_STRING support from an object file
> > +#
> 
> No licence..?
> 
Its BSD, same as the others, I let the README cover that.  We can include
it if we must, though we have lots of examples where we haven't bothered


> 
> > +# -------------------------------------------------------------------------
> > +import os
> > +import sys
> > +from optparse import OptionParser
> > +import string
> > +import json
> > +
> > +# For running from development directory. It should take precedence over the
> > +# installed pyelftools.
> > +sys.path.insert(0, '.')
> 
> Aside from causing all the subsequent imports to have PEP8 errors, this does
> not looks like a good way of pulling in project-specific Python library
> installs. Usual method is either using virtualenv or the PYTHONPATH
> enviornment variable.
> 
Nope, pep8 doesn't complain about this at all:
[nhorman at hmsreliant tools]$ pep8 ./pmdinfo.py 
pmdinfo.py:573:80: E501 line too long (80 > 79 characters)
[nhorman at hmsreliant tools]$ 

The line too long snuck in there recently on the last round of formatting errors


> 
> > +from elftools import __version__
> > +from elftools.common.exceptions import ELFError
> [..]
> > +from elftools.dwarf.constants import (
> > +    DW_LNS_copy, DW_LNS_set_file, DW_LNE_define_file)
> > +from elftools.dwarf.callframe import CIE, FDE
> 
> According to PyLint, most of these imports are unused.
Some of them are, the code was borrowed from some examples.

> 
> 
> > +
> > +
> > +class Vendor:
> 
> Old style class definition. Using modern notation it is:
> 
> class Vendor(object):
> 
> 
> > +    def report(self):
> > +        print "\t%s\t%s" % (self.ID, self.name)
> > +        for subID, subdev in self.subdevices.items():
> > +            subdev.report()
> 
> subID unused. An underscore can be used as a placeholder for these types of
> loops.
> 
> 
> > +    optparser.add_option("-t", "--table", dest="tblout",
> > +                         help="output information on hw support as a hex table",
> 
> PEP8: pmdinfo.py:573:80: E501 line too long (80 > 79 characters)
> 
As you note, none of these are catastrophic.  I'm willing to fix them up, but
given, the number of iterations I've gone through for minior nits, I would
prefer to see it incorporated before I post this series again.

Neil

> 
>