Hi Vishal, Thank you for your review! I'll send v2 patch.
- Masa On 06/12/2018 07:28 PM, Verma, Vishal L wrote: > On Mon, 2018-06-11 at 12:45 -0400, Masayoshi Mizuma wrote: >> From: Masayoshi Mizuma <m.miz...@jp.fujitsu.com> >> >> Include 'common' file to use some fucntions for test scripts. >> >> Signed-off-by: Masayoshi Mizuma <m.miz...@jp.fujitsu.com> >> --- >> test/blk-exhaust.sh | 21 +++------------- >> test/btt-check.sh | 35 ++++++++------------------ >> test/btt-errors.sh | 20 ++++----------- >> test/btt-pad-compat.sh | 52 ++++++++++++++------------------------- >> test/clear.sh | 21 +++------------- >> test/create.sh | 21 +++------------- >> test/daxdev-errors.sh | 19 +++----------- >> test/firmware-update.sh | 25 ++++--------------- >> test/inject-error.sh | 31 ++++++----------------- >> test/label-compat.sh | 21 +++------------- >> test/multi-dax.sh | 19 +++----------- >> test/pmem-errors.sh | 20 +++++---------- >> test/rescan-partitions.sh | 44 +++++++++------------------------ >> test/sector-mode.sh | 5 +--- >> 14 files changed, 89 insertions(+), 265 deletions(-) > > Hi Masayoshi, > > This is a welcome cleanup. It looks good except for a couple of nits. > First, can you add a SPDX style license header to the new test/common file? > Second, see below - > > [..] > >> >> diff --git a/test/firmware-update.sh b/test/firmware-update.sh >> index c2cf578..1ed60b1 100755 >> --- a/test/firmware-update.sh >> +++ b/test/firmware-update.sh >> @@ -12,25 +12,9 @@ rc=77 >> dev="" >> image="update-fw.img" >> >> -trap 'err $LINENO' ERR >> +. ./common >> >> -# $1: Line number >> -# $2: exit code >> -err() >> -{ >> - [ -n "$2" ] && rc="$2" >> - echo "test/firmware-update.sh: failed at line $1" >> - exit "$rc" >> -} >> - >> -check_min_kver() >> -{ >> - local ver="$1" >> - : "${KVER:=$(uname -r)}" >> - >> - [ -n "$ver" ] || return 1 >> - [[ "$ver" == "$(echo -e "$ver\n$KVER" | sort -V | head -1)" ]] >> -} >> +trap 'err $LINENO' ERR >> >> reset() >> { >> @@ -52,7 +36,7 @@ cleanup() >> detect() >> { >> dev=$($ndctl list -b "$bus" -D | jq .[0].dev | tr -d '"') >> - [ -n "$dev" ] || err "$LINENO" 2 >> + [ -n "$dev" ] || rc=2 && err "$LINENO" > > This || .. && will not work as expected. If $dev is null it will set rc to > 2, and if it exists, then it will err(). What we want is > > [ -n "$dev" ] || { rc=2 && err "$LINENO"; } > Or even better, > > @@ -36,7 +36,7 @@ cleanup() > detect() > { > dev=$($ndctl list -b "$bus" -D | jq .[0].dev | tr -d '"') > - [ -n "$dev" ] || rc=2 && err "$LINENO" > + [ -n "$dev" ] || err "$LINENO" > } > > do_tests() > @@ -50,6 +50,7 @@ check_min_kver "4.16" || do_skip "may lack firmware > update test handling" > modprobe nfit_test > rc=1 > reset > +rc=2 > detect > do_tests > > The rest looks good. > > Thanks, > -Vishal > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm