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
