From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <nhorman@tuxdriver.com>
Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])
 by dpdk.org (Postfix) with ESMTP id 8972D37B0
 for <dev@dpdk.org>; Sun, 27 May 2018 23:01:20 +0200 (CEST)
Received: from cpe-2606-a000-111b-40b7-215-ff-fecc-4872.dyn6.twc.com
 ([2606:a000:111b:40b7:215:ff:fecc:4872] helo=localhost)
 by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63)
 (envelope-from <nhorman@tuxdriver.com>)
 id 1fN2mv-0005Xn-0B; Sun, 27 May 2018 17:01:15 -0400
Date: Sun, 27 May 2018 17:00:39 -0400
From: Neil Horman <nhorman@tuxdriver.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org, john.mcnamara@intel.com, bruce.richardson@intel.com,
 Ferruh Yigit <ferruh.yigit@intel.com>,
 Stephen Hemminger <stephen@networkplumber.org>
Message-ID: <20180527210039.GA22397@neilslaptop.think-freely.org>
References: <20180115190545.25687-1-nhorman@tuxdriver.com>
 <20180214191916.7226-1-nhorman@tuxdriver.com>
 <1725451.L7JxISGBU9@xps>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1725451.L7JxISGBU9@xps>
User-Agent: Mutt/1.9.2 (2017-12-15)
X-Spam-Score: -2.9 (--)
X-Spam-Status: No
Subject: Re: [dpdk-dev] [PATCH v6] checkpatches.sh: Add checks for ABI
 symbol addition
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Sun, 27 May 2018 21:01:20 -0000

On Sun, May 27, 2018 at 09:34:13PM +0200, Thomas Monjalon wrote:
> Hi Neil,
> 
> Sorry, this patch has been forgotten during the whole release cycle.
> 
Its ok, though that is frustrating, as we now need to re-familiarize ourselves
with how this patch works.

> I see an issue about the dependency on filterdiff.
> Is there a way to avoid it?
> 
I suppose, but to do so would require some awk magic.  Is there any advantage to
having an awk dependency rather than a filterdiff dependency?  filterdiff is a
pretty universal tool.

> I have some minor comments below.
> 
> 14/02/2018 20:19, Neil Horman:
> > @@ -61,6 +65,7 @@ print_usage () {
> >  	END_OF_HELP
> >  }
> >  
> > +
> 
> This new blank line is probably a leftover.
Yeah, if I wind up respinning this, I'll remove it.

> 
> >  number=0
> >  quiet=false
> >  verbose=false
> > @@ -86,19 +91,50 @@ total=0
> >  status=0
> >  
> >  check () { # <patch> <commit> <title>
> > +	local ret=0
> > +	TMPINPUT=`mktemp checkpatches.XXXXXX`
> 
> It is preferred to use $() construct to be consistent in the file.
> 
> > +
> >  	total=$(($total + 1))
> >  	! $verbose || printf '\n### %s\n\n' "$3"
> >  	if [ -n "$1" ] ; then
> >  		report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
> >  	elif [ -n "$2" ] ; then
> > -		report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
> > +		git format-patch --find-renames --no-stat --stdout -1 $commit > ./$TMPINPUT
> > +		report=$(cat ./$TMPINPUT |
> >  			$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> 
> No need to use cat here.
> 
Actually, I vaguely recall with these changes I did encounter a problem with passing
output to checkpatch, but its been so long I can't recall now.

> >  	else
> > -		report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > +		cat > ./$TMPINPUT
> > +		report=$(cat ./$TMPINPUT | $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> > +	fi
> 
> All calls to DPDK_CHECKPATCH_PATH can be moved to one call after the "fi".
> Just need to set TPMINPUT=$1 in first case (and not remove TPMINPUT
> in this case).
> 
Sure.

> > +	if [ $? -ne 0 ]
> > +	then
> 
> For consistency, the style in this file is:
> 	if [ $? -ne 0 ] ; then
> 
> > +		$verbose || printf '\n### %s\n\n' "$3"
> > +		printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> > +		ret=1
> > +	fi
> > +
> > +	! $verbose || printf '\nChecking API additions/removals:\n'
> > +
> > +	if [ -n "$1" ] ; then
> > +		report=$($VALIDATE_NEW_API "$1")
> > +	elif [ -n "$2" ] ; then
> > +		report=$( cat ./$TMPINPUT | 
> > +			$VALIDATE_NEW_API -)
> > +	else
> > +		report=$(cat ./$TMPINPUT | $VALIDATE_NEW_API -)
> > +	fi
> 
> Same as above, it can be simplified by only one line for all cases.

Fine, at this point, I don't know when I'll get to this though, its pretty busy
here at the moment.

> 
> 
> 
>