From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id BA0908E6E for ; Thu, 19 Apr 2018 19:52:29 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4E767722ED; Thu, 19 Apr 2018 17:52:29 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.61]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E40472166BAE; Thu, 19 Apr 2018 17:52:28 +0000 (UTC) From: Aaron Conole To: Arnon Warshavsky Cc: thomas@monjalon.net, anatoly.burakov@intel.com, wenzhuo.lu@intel.com, declan.doherty@intel.com, jerin.jacob@caviumnetworks.com, bruce.richardson@intel.com, ferruh.yigit@intel.com, dev@dpdk.org References: <1524117669-25729-1-git-send-email-arnon@qwilt.com> <1524117669-25729-12-git-send-email-arnon@qwilt.com> Date: Thu, 19 Apr 2018 13:52:28 -0400 In-Reply-To: <1524117669-25729-12-git-send-email-arnon@qwilt.com> (Arnon Warshavsky's message of "Thu, 19 Apr 2018 09:01:09 +0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 19 Apr 2018 17:52:29 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 19 Apr 2018 17:52:29 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'aconole@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v4 11/11] devtools: prevent new instances of rte_panic and rte_exit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Apr 2018 17:52:30 -0000 Arnon Warshavsky writes: > 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 > --- > devtools/checkpatches.sh | 94 +++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 93 insertions(+), 1 deletion(-) > > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh > index 7676a6b..fb37838 100755 > --- a/devtools/checkpatches.sh > +++ b/devtools/checkpatches.sh > @@ -61,6 +61,90 @@ print_usage () { > END_OF_HELP > } > > +check_forbidden_additions() { # > + # --------------------------------- > + #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){ > + print "Forbidden expressions in " substr(last_file,6) ":" > + for (key in expressions) { > + if (expressions[key] > 0) { > + print key > + } > + } > + exit 1 > + } > + } > +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\\\(" \ I don't think rte_panic should be considered forbidden. Rather their use should be flagged (as this patch does). However, the 'exit 1' (which will return a failure for the automatic checkpatch script bot) might end up problematic as maintainers might consider it a patch that is not ready. I wouldn't object to this patch, but just think you might want to change the print to something like: WARN: Are you sure you meant to use "$expression"? > + "$awk_script" - > +} > + > number=0 > quiet=false > verbose=false > @@ -89,11 +173,19 @@ check () { # > 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