On Thu, 2018-07-12 at 15:51 -0400, Masayoshi Mizuma wrote:
> 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...

.. and within the $(( )), you can forego the '$' for variables. So it
becomes: i=$((i + 1))

> 
> > +   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
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Reply via email to