Re: [ndctl PATCH v11 5/5] ndctl, test: add a new unit test for monitor

["Verma, Vishal L" <vishal.l.verma@intel.com>]

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