Re: [dpdk-dev] [PATCH v8] checkpatches.sh: Add checks for ABI symbol addition

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Jun 26, 2018 at 01:04:16AM +0200, Thomas Monjalon wrote:
> 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.
> 
I prefer echo, just because its what I've always used.  I'm open to changing it
if there is a compelling reason to do so.

> [...]
> > +exit $exit_code
> > +
> > +
> 
> Ironically, this patch doesn't pass checkpatch test because of
> the trailing new lines.
> 
Patchwork reports only a single error:
https://patches.dpdk.org/patch/41139/

Is the CI checkpatch different from the checkpatch we provide here?  If so, why?

> [...]
> > +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).
> 
Sure

> What the grep is supposed to match?
The name of the temp file created.  TMPINPUT can take on two classes of values:
1) The name of a passed in patch file
2) The name of a temp file created to hold a patch streamed in on stdin

when we clean up on exit, we want to delete class 2, but never class 1, so we
match on the temp file root name 'checkpatch'

> (side note, there is a typo: checkpaches -> checkpatches)
will fix

> Is it to remove file only in case of mktemp?
yes

> I think it is a risky pattern matching. I suggest '^checkpatches\.'
fine

> 
> > +	if [ $? -eq 0 ]; then
> 
> Could be easier to read if combining "if" and "grep":
> 	if echo $tmpinput | grep -q '^checkpatches\.' ; then
That seems fairly out of line with the style of the rest of the file

> 
> > +		rm -f $TMPINPUT
> > +	fi
> > +}
> 
> [...]
> > +		TMPINPUT=$(mktemp checkpatches.XXXXXX)
> 
> Open to discussion: do we prefer local dir or /tmp?
> Some tools are using /tmp.
> 
I don't think thats our call.  If an end user wants to specify the location of
temp files, they can do so by setting $TMPDIR.  We don't need to mandate it.

> [...]
> > +	report=$($DPDK_CHECKPATCH_PATH $options $TMPINPUT 2>/dev/null)
> > +
> 
> Please, no blank line between command and test.
> 
very well

> > +	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?
> 
Because it was in keeping with the style that I was working with.  I can revert
it

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