From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 72667CE7 for ; Wed, 29 Jun 2016 18:18:30 +0200 (CEST) Received: from [2606:a000:111b:40ed:7aac:c0ff:fec2:933b] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1bIIC1-0005XH-Te; Wed, 29 Jun 2016 12:18:27 -0400 Date: Wed, 29 Jun 2016 12:18:20 -0400 From: Neil Horman To: Remy Horton Cc: dev@dpdk.org, Bruce Richardson , Thomas Monjalon , Stephen Hemminger , Panu Matilainen Message-ID: <20160629161820.GB17663@hmsreliant.think-freely.org> References: <1465494421-6210-1-git-send-email-nhorman@tuxdriver.com> <1466189185-21952-1-git-send-email-nhorman@tuxdriver.com> <1466189185-21952-6-git-send-email-nhorman@tuxdriver.com> <54616833-c0cc-8e93-c9de-1580f264c57e@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54616833-c0cc-8e93-c9de-1580f264c57e@intel.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Score: -1.0 (-) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCHv8 5/6] pmdinfo.py: Add tool to query binaries for hw and other support information X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jun 2016 16:18:30 -0000 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@hmsreliant tools]$ pep8 ./pmdinfo.py pmdinfo.py:573:80: E501 line too long (80 > 79 characters) [nhorman@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 > >