Hi Qi,

Nice work! Let me ask some comments.

On 07/10/2018 11:00 PM, QI Fuli wrote:
[...]
> diff --git a/test/monitor.sh b/test/monitor.sh
> new file mode 100755
> index 0000000..43cb11f
> --- /dev/null
> +++ b/test/monitor.sh
> @@ -0,0 +1,177 @@
> +#!/bin/bash -Ex
> +
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright(c) 2018, FUJITSU LIMITED. All rights reserved.
> +
> +rc=77
> +logfile=""
> +conf_file=""
> +monitor_dimms=""
> +monitor_regions=""
> +monitor_namespace=""
> +monitor_pid=65536
> +
> +. ./common
> +
> +trap 'err $LINENO' ERR
> +
> +check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor 
> service"
> +
> +init()
> +{
> +     $NDCTL disable-region -b $NFIT_TEST_BUS0 all
> +     $NDCTL zero-labels -b $NFIT_TEST_BUS0 all
> +     $NDCTL enable-region -b $NFIT_TEST_BUS0 all
> +}
> +
> +start_monitor()
> +{
> +     logfile=$(mktemp)
> +     $NDCTL monitor -l $logfile $1 &
> +     monitor_pid=$!

> +     truncate --size 0 $logfile #remove startup log
> +     sync; sleep 3

Should this sync be moved to before the truncate?

> +}
> +
> +get_monitor_dimm()
> +{
> +     jlist=$(./list-smart-dimm -b $smart_supported_bus $1)
> +     monitor_dimms=$(jq '.[]."dev"?, ."dev"?' <<<$jlist | sort | uniq | 
> xargs)
> +}
> +
> +call_notify()
> +{
> +     ./smart-notify $smart_supported_bus
> +     sync; sleep 3
> +}
> +
> +inject_smart()
> +{
> +     $NDCTL inject-smart $monitor_dimms $1
> +     sync; sleep 3
> +}
> +
> +check_result()
> +{
> +     jlog=$(cat $logfile)
> +     notify_dimms=$(jq ."dimm"."dev" <<<$jlog | sort | uniq | xargs)
> +     [[ $monitor_dimms == $notify_dimms ]]
> +}
> +
> +stop_monitor()
> +{
> +     kill $monitor_pid
> +     rm $logfile
> +}
> +
> +create_conf_file()
> +{
> +     conf_file=$(mktemp)
> +     echo "dimm = $1" > $conf_file
> +}
> +

> +test_filter_dimm()
> +{
> +     smart_supported_bus=$NFIT_TEST_BUS0
> +     monitor_dimms=$(./list-smart-dimm -b $smart_supported_bus | jq -r 
> .[0].dev)
> +     if [ -z $monitor_dimms ]; then
> +             smart_supported_bus=$NFIT_TEST_BUS1
> +             monitor_dimms=$(./list-smart-dimm -b $smart_supported_bus | jq 
> -r .[0].dev)
> +     fi
> +     start_monitor "-d $monitor_dimms"
> +     call_notify
> +     check_result
> +     stop_monitor
> +}

I think the global variable "smart_supported_bus" configuration
should be separated from this function.
Like as:

set_smart_supported_bus()
{
        smart_supported_bus=$NFIT_TEST_BUS0
        monitor_dimms=$(./list-smart-dimm -b $smart_supported_bus | jq -r 
.[0].dev)
        if [ -z $monitor_dimms ]; then
                smart_supported_bus=$NFIT_TEST_BUS1
        fi
}

> +
> +test_filter_bus()
> +{
> +     monitor_dimms=""
> +     get_monitor_dimm
> +     start_monitor "-b $smart_supported_bus"
> +     call_notify
> +     check_result
> +     stop_monitor
> +}
> +
> +test_filter_region()
> +{
> +     monitor_dimms=""
> +     monitor_region=""
> +     count=$($NDCTL list -R -b $smart_supported_bus | jq -r .[].dev | wc -l)
> +     i=0
> +     while [ $i -lt $count ]; do
> +             monitor_region=$($NDCTL list -R -b $smart_supported_bus | jq -r 
> .[$i].dev)
> +             get_monitor_dimm "-r $monitor_region"
> +             [ ! -z $monitor_dimms ] && break

> +             i=$[$i+1]

i=$(( $i + 1 )) is better. $[...] is not described in the man page of bash...

> +     done
> +     start_monitor "-r $monitor_region"
> +     call_notify
> +     check_result
> +     stop_monitor
> +}
> +
> +test_filter_namespace()
> +{
> +     monitor_dimms=""
> +     init
> +     monitor_namespace=$($NDCTL create-namespace -b $smart_supported_bus | 
> jq -r .dev)
> +     get_monitor_dimm "-n $monitor_namespace"
> +     start_monitor "-n $monitor_namespace"
> +     call_notify
> +     check_result
> +     stop_monitor
> +     $NDCTL destroy-namespace $monitor_namespace -f
> +}
> +
> +test_conf_file()
> +{
> +     create_conf_file  "$monitor_dimms"
> +     start_monitor "-c $conf_file"
> +     call_notify
> +     check_result
> +     stop_monitor
> +     rm $conf_file
> +}
> +
> +test_filter_dimmevent()
> +{
> +     monitor_dimms="$(echo $monitor_dimms | awk '{print $1}')"
> +
> +     start_monitor "-d $monitor_dimms -D dimm-unclean-shutdown"
> +     inject_smart "-U"
> +     check_result
> +     stop_monitor
> +
> +     inject_value=$($NDCTL list -H -d $monitor_dimms | jq -r 
> ."health"."spares_threshold")

> +     inject_value=$[$inject_value-1]

Same as above.

> +     start_monitor "-d $monitor_dimms -D dimm-spares-remaining"
> +     inject_smart "-s $inject_value"
> +     check_result
> +     stop_monitor
> +
> +     inject_value=$($NDCTL list -H -d $monitor_dimms | jq -r 
> ."health"."temperature_threshold")

> +     inject_value=$[$inject_value+1]

Same as above.

Thanks,
Masa

> +     start_monitor "-d $monitor_dimms -D dimm-media-temperature"
> +     inject_smart "-s $inject_value"
> +     check_result
> +     stop_monitor
> +}
> +
> +do_tests()
> +{
> +     test_filter_dimm
> +     test_filter_bus
> +     test_filter_region
> +     test_filter_namespace
> +     test_conf_file
> +     test_filter_dimmevent
> +}
> +
> +modprobe nfit_test
> +rc=1
> +init
> +do_tests
> +_cleanup
> +exit 0
> 
_______________________________________________
Linux-nvdimm mailing list
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm

Reply via email to