[dpdk-dev,v8,10/10] devtools: prevent 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>
---
devtools/checkpatches.sh | 95 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 94 insertions(+), 1 deletion(-)
Comments
The purpose of this patch series is to cleanup the library code
from paths that end up aborting the process,
and move to checking error values, in order to allow the running process
perform an orderly teardown or other mitigation of the event.
This patch modifies the majority of rte_panic calls
under lib and drivers, and replaces them with a log message
and an error return code according to context,
that can be propagated up the call stack.
- Focus was given to the dpdk initialization path
- Some of the panic calls within drivers were left in place where
the call is from within an interrupt or calls that are
on the data path,where there is no simple applicative
route to propagate the error to temination.
These should be handled by the driver maintainers..
- local void functions with no api were changed to retrun a value
where needed
- No change took place in example and test files
- No change took place for debug assertions calling panic
- A new function was added to devtools/checkpatches.sh
in order to prevent new additions of calls to rte_panic
under lib and drivers.
Keep calm and don't panic
---
v2:
- reformat error messages so that literal string are in the same line
- fix typo in commit message
- add new return code to doxigen of rte_memzone_free()
v3:
- submit all 13 patches changed and unchanged in the same patchset
v4:
- remove 2 patches that are no more relevant
- fix split literal string in error message
- change return value -1 to enum
- split value and success code in a static function
v5:
- reword commit messages
- revert thread related instances back to panicing
- handle file descriptors with state to reset after eal init failure
in case re initialization takes place
v6:
- Use pmd log macro rather than rte_log
v7:
- use bond specific , dpaa2 specific and eventdev specific log macros
v8:
- Seperate the 2 drivers salad back to distinct bond and dpaa patches
- Add missing file descriptor closing when returnning an error
- Remove half baked thread patch to be handled in the next version
- Remove duplicate function call after rebase
v9:
- Add missing file descriptor closing when retruning with error
Arnon Warshavsky (10):
crypto/dpaa: replace rte_panic instances in crypto/dpaa driver
bond: replace rte_panic instances in bonding driver
e1000: replace rte_panic instances in e1000 driver
ixgbe: replace rte_panic instances in ixgbe driver
eal: replace rte_panic instances in eventdev
kni: replace rte_panic instances in kni
eal: replace rte_panic instances in hugepage_info
eal: replace rte_panic instances in ethdev
eal: replace rte_panic instances in init sequence
devtools: prevent new instances of rte_panic and rte_exit
devtools/checkpatches.sh | 95 +++++++++++++++++++-
drivers/crypto/dpaa2_sec/dpaa2_sec_dpseci.c | 8 +-
drivers/crypto/dpaa_sec/dpaa_sec.c | 10 ++-
drivers/net/bonding/rte_eth_bond_8023ad.c | 29 ++++---
drivers/net/bonding/rte_eth_bond_8023ad_private.h | 2 +-
drivers/net/bonding/rte_eth_bond_api.c | 22 +++--
drivers/net/bonding/rte_eth_bond_pmd.c | 9 +-
drivers/net/bonding/rte_eth_bond_private.h | 2 +-
drivers/net/e1000/e1000_ethdev.h | 2 +-
drivers/net/e1000/igb_ethdev.c | 4 +-
drivers/net/e1000/igb_pf.c | 15 ++--
drivers/net/ixgbe/ixgbe_ethdev.c | 6 +-
drivers/net/ixgbe/ixgbe_ethdev.h | 2 +-
drivers/net/ixgbe/ixgbe_pf.c | 15 ++--
lib/librte_eal/bsdapp/eal/eal.c | 71 +++++++++++----
lib/librte_eal/linuxapp/eal/eal.c | 101 +++++++++++++++-------
lib/librte_eal/linuxapp/eal/eal_hugepage_info.c | 37 +++++---
lib/librte_ether/rte_ethdev.c | 42 ++++++---
lib/librte_ether/rte_ethdev.h | 4 +-
lib/librte_eventdev/rte_eventdev_pmd_pci.h | 8 +-
lib/librte_eventdev/rte_eventdev_pmd_vdev.h | 8 +-
lib/librte_kni/rte_kni.c | 18 ++--
lib/librte_kni/rte_kni_fifo.h | 11 ++-
23 files changed, 385 insertions(+), 136 deletions(-)
26/04/2018 08:20, Arnon Warshavsky:
> The purpose of this patch series is to cleanup the library code
> from paths that end up aborting the process,
> and move to checking error values, in order to allow the running process
> perform an orderly teardown or other mitigation of the event.
>
> This patch modifies the majority of rte_panic calls
> under lib and drivers, and replaces them with a log message
> and an error return code according to context,
> that can be propagated up the call stack.
>
> - Focus was given to the dpdk initialization path
> - Some of the panic calls within drivers were left in place where
> the call is from within an interrupt or calls that are
> on the data path,where there is no simple applicative
> route to propagate the error to temination.
> These should be handled by the driver maintainers..
> - local void functions with no api were changed to retrun a value
> where needed
> - No change took place in example and test files
> - No change took place for debug assertions calling panic
> - A new function was added to devtools/checkpatches.sh
> in order to prevent new additions of calls to rte_panic
> under lib and drivers.
>
> Keep calm and don't panic
What is the status of this patchset?
It seems not ready for RC1 (today).
I don't want to push it in RC2 because it changes too many things.
So it's today or wait for 18.08. What do you think?
The last patch for check tooling can be separated and pushed at anytime.
26/04/2018 08:20, Arnon Warshavsky:
> v8:
> - Seperate the 2 drivers salad back to distinct bond and dpaa patches
> - Add missing file descriptor closing when returnning an error
> - Remove half baked thread patch to be handled in the next version
> - Remove duplicate function call after rebase
You forgot the ack from Stephen on the series
On Fri, Apr 27, 2018, 17:22 Thomas Monjalon <thomas@monjalon.net> wrote:
> 26/04/2018 08:20, Arnon Warshavsky:
> > The purpose of this patch series is to cleanup the library code
> > from paths that end up aborting the process,
> > and move to checking error values, in order to allow the running process
> > perform an orderly teardown or other mitigation of the event.
> >
> > This patch modifies the majority of rte_panic calls
> > under lib and drivers, and replaces them with a log message
> > and an error return code according to context,
> > that can be propagated up the call stack.
> >
> > - Focus was given to the dpdk initialization path
> > - Some of the panic calls within drivers were left in place where
> > the call is from within an interrupt or calls that are
> > on the data path,where there is no simple applicative
> > route to propagate the error to temination.
> > These should be handled by the driver maintainers..
> > - local void functions with no api were changed to retrun a value
> > where needed
> > - No change took place in example and test files
> > - No change took place for debug assertions calling panic
> > - A new function was added to devtools/checkpatches.sh
> > in order to prevent new additions of calls to rte_panic
> > under lib and drivers.
> >
> > Keep calm and don't panic
>
> What is the status of this patchset?
> It seems not ready for RC1 (today).
> I don't want to push it in RC2 because it changes too many things.
> So it's today or wait for 18.08. What do you think?
>
> The last patch for check tooling can be separated and pushed at anytime.
>
Yes,unfortunately its 18.08.I am away the entire weekend from any means of
getting the code fixed, so only on Sunday I will split the last patch from
the rest.
Should I put the tooling patch as a new set and refer both patchsets to the
current v9?
>
>
>
27/04/2018 18:31, Arnon Warshavsky:
> On Fri, Apr 27, 2018, 17:22 Thomas Monjalon <thomas@monjalon.net> wrote:
> > What is the status of this patchset?
> > It seems not ready for RC1 (today).
> > I don't want to push it in RC2 because it changes too many things.
> > So it's today or wait for 18.08. What do you think?
> >
> > The last patch for check tooling can be separated and pushed at anytime.
> >
>
> Yes,unfortunately its 18.08.I am away the entire weekend from any means of
> getting the code fixed, so only on Sunday I will split the last patch from
> the rest.
So you are OK with considering this patchset for 18.08?
And take the tool in 18.05?
> Should I put the tooling patch as a new set and refer both patchsets to the
> current v9?
Yes you can split and keep v9 numbering for both.
On Fri, Apr 27, 2018, 19:40 Thomas Monjalon <thomas@monjalon.net> wrote:
> 27/04/2018 18:31, Arnon Warshavsky:
> > On Fri, Apr 27, 2018, 17:22 Thomas Monjalon <thomas@monjalon.net> wrote:
> > > What is the status of this patchset?
> > > It seems not ready for RC1 (today).
> > > I don't want to push it in RC2 because it changes too many things.
> > > So it's today or wait for 18.08. What do you think?
> > >
> > > The last patch for check tooling can be separated and pushed at
> anytime.
> > >
> >
> > Yes,unfortunately its 18.08.I am away the entire weekend from any means
> of
> > getting the code fixed, so only on Sunday I will split the last patch
> from
> > the rest.
>
> So you are OK with considering this patchset for 18.08?
> And take the tool in 18.05?
>
> Yes, it looks like the best option here.
Thanks
/Arnon
>
>
>
@@ -61,6 +61,91 @@ 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){
+ print "Warning: 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 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\\\(" \
+ "$awk_script" -
+}
+
number=0
quiet=false
verbose=false
@@ -89,11 +174,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