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

["Qi, Fuli" <qi.fuli@jp.fujitsu.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))
> 

Ok, I will fix it.

Thanks Vishal.
Thanks Masa.
QI

> >
> > > +	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