From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v8] checkpatches.sh: Add checks for ABI
	symbol addition
Date: Tue, 26 Jun 2018 01:04:16 +0200
Message-ID: <2500803.CjdbPassZA@xps>
References: <20180115190545.25687-1-nhorman@tuxdriver.com>
 <20180614133020.15604-1-nhorman@tuxdriver.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, john.mcnamara@intel.com, bruce.richardson@intel.com,
 Ferruh Yigit <ferruh.yigit@intel.com>,
 Stephen Hemminger <stephen@networkplumber.org>
To: Neil Horman <nhorman@tuxdriver.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com
 [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id 5148A37A2
 for <dev@dpdk.org>; Tue, 26 Jun 2018 01:04:19 +0200 (CEST)
In-Reply-To: <20180614133020.15604-1-nhorman@tuxdriver.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

14/06/2018 15:30, Neil Horman:
>  * found a way to eliminate the use of filterdiff (new awk rules)

Thanks a lot for not requiring filterdiff dependency.

[...]
> +				# Just inform the user of this occurrence, but
> +				# don't flag it as an error
> +				echo -n "INFO: symbol $syname is added but "
> +				echo -n "patch has insuficient context "
> +				echo -n "to determine the section name "
> +				echo -n "please ensure the version is "
> +				echo "EXPERIMENTAL"

For info, I think nowadays "printf" is preferred over "echo -n"
But if you prefer "echo -n" for any reason, no problem.

[...]
> +exit $exit_code
> +
> +

Ironically, this patch doesn't pass checkpatch test because of
the trailing new lines.

[...]
> +clean_tmp_files() {
> +	echo $TMPINPUT | grep -q checkpaches

Two comments here.

Since TMPINPUT is not supposed to be overwritten by environment,
I think it is better to make it lowercase (kind of convention).

What the grep is supposed to match?
(side note, there is a typo: checkpaches -> checkpatches)
Is it to remove file only in case of mktemp?
I think it is a risky pattern matching. I suggest '^checkpatches\.'

> +	if [ $? -eq 0 ]; then

Could be easier to read if combining "if" and "grep":
	if echo $tmpinput | grep -q '^checkpatches\.' ; then

> +		rm -f $TMPINPUT
> +	fi
> +}

[...]
> +		TMPINPUT=$(mktemp checkpatches.XXXXXX)

Open to discussion: do we prefer local dir or /tmp?
Some tools are using /tmp.

[...]
> +	report=$($DPDK_CHECKPATCH_PATH $options $TMPINPUT 2>/dev/null)
> +

Please, no blank line between command and test.

> +	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'
> +
> +	report=$($VALIDATE_NEW_API "$TMPINPUT")
> +

Same comments about blank lines.

> +	if [ $? -ne 0 ]; then
> +		printf '%s\n' "$report"
> +		ret=1
> +	fi
> +
> +	clean_tmp_files
> +	if [ $ret -eq 0 ]; then
> +		return 0
>  	fi
> -	[ $? -ne 0 ] || return 0

Why replacing this oneliner by a longer "if" block?

After this review, I think I won't have any more comment.
Thanks Neil