[dpdk-dev,v10] devtools: alert on new instances of rte_panic and rte_exit
Checks
Commit Message
This patch adds a new function that is called
per every checked patch,
and alerts for new instances of rte_panic/rte_exit.
The check excludes comments, and alerts in the case
of a positive balance between additions and removals.
Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
---
devtools/checkpatches.sh | 96 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 95 insertions(+), 1 deletion(-)
Comments
On 04/30/2018 07:45 AM, Arnon Warshavsky wrote:
> This patch adds a new function that is called
> per every checked patch,
> and alerts for new instances of rte_panic/rte_exit.
> The check excludes comments, and alerts in the case
> of a positive balance between additions and removals.
>
> Signed-off-by: Arnon Warshavsky <arnon@qwilt.com>
> Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
Tested-by: Kevin Traynor <ktraynor@redhat.com>
> ---
> devtools/checkpatches.sh | 96 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 7676a6b..c7d927b 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -61,6 +61,92 @@ print_usage () {
> END_OF_HELP
> }
>
> +check_forbidden_additions() { # <file>
> + # ---------------------------------
> + #This awk script receives a list of expressions to monitor
> + #and a list of folders to search these expressions in
> + # - No search is done inside comments
> + # - Both additions and removals of the expressions are checked
> + # A positive balance of additions fails the check
> + read -d '' awk_script << 'EOF'
> + BEGIN{
> + split(FOLDERS,deny_folders," ");
> + split(EXPRESSIONS,deny_expr," ");
> + in_file=0;
> + in_comment=0;
> + count=0;
> + comment_start="/*"
> + comment_end="*/"
> + }
> + # search for add/remove instances in current file
> + # state machine assumes the comments structure is enforced by
> + # checkpatches.pl
> + (in_file) {
> + # comment start
> + if (index($0,comment_start) > 0){
> + in_comment = 1
> + }
> + # non comment code
> + if (in_comment == 0) {
> + for (i in deny_expr) {
> + forbidden_added = "^\+.*" deny_expr[i];
> + forbidden_removed="^-.*" deny_expr[i];
> + current = expressions[deny_expr[i]]
> + if ($0 ~ forbidden_added) {
> + count = count + 1;
> + expressions[deny_expr[i]] = current + 1
> + }
> + if ($0 ~ forbidden_removed) {
> + count = count - 1;
> + expressions[deny_expr[i]] = current - 1
> + }
> + }
> + }
> +
> + # comment end
> + if (index($0,comment_end) > 0) {
> + in_comment = 0
> + }
> + }
> + # switch to next file , check if the balance of add/remove
> + # of previous filehad new additions
> + ($0 ~ "^\+\+\+ b/") {
> + in_file = 0;
> + if (count > 0){
> + exit;
> + }
> + for (i in deny_folders){
> + re = "^\+\+\+ b/" deny_folders[i];
> + if ($0 ~ deny_folders[i]) {
> + in_file = 1
> + last_file = $0
> + }
> + }
> + }
> + END{
> + if (count > 0){
> + warnText = "\\033[1;31m" "Warning:" "\\033[0m"
> + print warnText " in " substr(last_file,6) ":"
> + print "are you sure you want to add the following:"
> + for (key in expressions) {
> + if (expressions[key] > 0) {
> + print key
> + }
> + }
> + exit 0
> + }
> + }
> +EOF
> + # ---------------------------------
> +
> + # refrain from new additions of rte_panic() and rte_exit()
> + # under lib and net
> + # multiple folders and expressions are separated by spaces
> + awk -v FOLDERS="lib net" \
> + -v EXPRESSIONS="rte_panic\\\( rte_exit\\\(" \
> + "$awk_script" -
> +}
> +
> number=0
> quiet=false
> verbose=false
> @@ -89,11 +175,19 @@ check () { # <patch> <commit> <title>
> total=$(($total + 1))
> ! $verbose || printf '\n### %s\n\n' "$3"
> if [ -n "$1" ] ; then
> + cat "$1" | check_forbidden_additions
> + [ $? -eq 0 ] || return 0
> report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
> elif [ -n "$2" ] ; then
> - report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
> + params=$(echo "--find-renames --no-stat --stdout -1")
> + body=$(git format-patch $params $commit)
> + echo "$body" | check_forbidden_additions
> + [ $? -eq 0 ] || return 0
> + report=$(echo "$body" |
> $DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> else
> + check_forbidden_additions -
> + [ $? -eq 0 ] || return 0
> report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> fi
> [ $? -ne 0 ] || return 0
>
Hi,
30/04/2018 08:45, Arnon Warshavsky:
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> + check_forbidden_additions -
This is consuming stdin.
I guess the checkpatch.pl will have nothing to check in the next step.
We should merge Neil's patch first, because he is adding a tmpfile
to solve the issue of stdin read only once.
> + [ $? -eq 0 ] || return 0
This test looks reversed: if the result is not 0, we should not return 0.
And by the way, I think it is better to continue with other checks.
Neil's patch is setting ret=1, continue and return at the end.
> report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
This is consuming stdin.
> I guess the checkpatch.pl will have nothing to check in the next step.
> We should merge Neil's patch first, because he is adding a tmpfile
> to solve the issue of stdin read only once.
>
Yup. Missed the fact stdin is consumed. temp file is a good idea
>
> > + [ $? -eq 0 ] || return 0
>
> This test looks reversed: if the result is not 0, we should not return 0.
> And by the way, I think it is better to continue with other checks.
> Neil's patch is setting ret=1, continue and return at the end.
>
> > report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
>
As it was agreed that this check should only warn and not fail the tests,
The actual situation is that the function check_forbidden_additions()
always returns zero.
Once Neils patch is in, I can align to the return value propagation,
but the current code never fails (that may change of course for future
different tokens)
thanks
/Arnon
27/05/2018 22:34, Arnon Warshavsky:
> This is consuming stdin.
> > I guess the checkpatch.pl will have nothing to check in the next step.
> > We should merge Neil's patch first, because he is adding a tmpfile
> > to solve the issue of stdin read only once.
> >
>
> Yup. Missed the fact stdin is consumed. temp file is a good idea
>
> >
> > > + [ $? -eq 0 ] || return 0
> >
> > This test looks reversed: if the result is not 0, we should not return 0.
> > And by the way, I think it is better to continue with other checks.
> > Neil's patch is setting ret=1, continue and return at the end.
> >
> > > report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> >
>
> As it was agreed that this check should only warn and not fail the tests,
> The actual situation is that the function check_forbidden_additions()
> always returns zero.
> Once Neils patch is in, I can align to the return value propagation,
> but the current code never fails (that may change of course for future
> different tokens)
Neil's patch is merged. Please would you like to resume your work?
@@ -61,6 +61,92 @@ print_usage () {
END_OF_HELP
}
+check_forbidden_additions() { # <file>
+ # ---------------------------------
+ #This awk script receives a list of expressions to monitor
+ #and a list of folders to search these expressions in
+ # - No search is done inside comments
+ # - Both additions and removals of the expressions are checked
+ # A positive balance of additions fails the check
+ read -d '' awk_script << 'EOF'
+ BEGIN{
+ split(FOLDERS,deny_folders," ");
+ split(EXPRESSIONS,deny_expr," ");
+ in_file=0;
+ in_comment=0;
+ count=0;
+ comment_start="/*"
+ comment_end="*/"
+ }
+ # search for add/remove instances in current file
+ # state machine assumes the comments structure is enforced by
+ # checkpatches.pl
+ (in_file) {
+ # comment start
+ if (index($0,comment_start) > 0){
+ in_comment = 1
+ }
+ # non comment code
+ if (in_comment == 0) {
+ for (i in deny_expr) {
+ forbidden_added = "^\+.*" deny_expr[i];
+ forbidden_removed="^-.*" deny_expr[i];
+ current = expressions[deny_expr[i]]
+ if ($0 ~ forbidden_added) {
+ count = count + 1;
+ expressions[deny_expr[i]] = current + 1
+ }
+ if ($0 ~ forbidden_removed) {
+ count = count - 1;
+ expressions[deny_expr[i]] = current - 1
+ }
+ }
+ }
+
+ # comment end
+ if (index($0,comment_end) > 0) {
+ in_comment = 0
+ }
+ }
+ # switch to next file , check if the balance of add/remove
+ # of previous filehad new additions
+ ($0 ~ "^\+\+\+ b/") {
+ in_file = 0;
+ if (count > 0){
+ exit;
+ }
+ for (i in deny_folders){
+ re = "^\+\+\+ b/" deny_folders[i];
+ if ($0 ~ deny_folders[i]) {
+ in_file = 1
+ last_file = $0
+ }
+ }
+ }
+ END{
+ if (count > 0){
+ warnText = "\\033[1;31m" "Warning:" "\\033[0m"
+ print warnText " in " substr(last_file,6) ":"
+ print "are you sure you want to add the following:"
+ for (key in expressions) {
+ if (expressions[key] > 0) {
+ print key
+ }
+ }
+ exit 0
+ }
+ }
+EOF
+ # ---------------------------------
+
+ # refrain from new additions of rte_panic() and rte_exit()
+ # under lib and net
+ # multiple folders and expressions are separated by spaces
+ awk -v FOLDERS="lib net" \
+ -v EXPRESSIONS="rte_panic\\\( rte_exit\\\(" \
+ "$awk_script" -
+}
+
number=0
quiet=false
verbose=false
@@ -89,11 +175,19 @@ check () { # <patch> <commit> <title>
total=$(($total + 1))
! $verbose || printf '\n### %s\n\n' "$3"
if [ -n "$1" ] ; then
+ cat "$1" | check_forbidden_additions
+ [ $? -eq 0 ] || return 0
report=$($DPDK_CHECKPATCH_PATH $options "$1" 2>/dev/null)
elif [ -n "$2" ] ; then
- report=$(git format-patch --find-renames --no-stat --stdout -1 $commit |
+ params=$(echo "--find-renames --no-stat --stdout -1")
+ body=$(git format-patch $params $commit)
+ echo "$body" | check_forbidden_additions
+ [ $? -eq 0 ] || return 0
+ report=$(echo "$body" |
$DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
else
+ check_forbidden_additions -
+ [ $? -eq 0 ] || return 0
report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
fi
[ $? -ne 0 ] || return 0