* [ndctl PATCH v3] ndctl, test: add a new unit test for monitor @ 2018-07-07 5:35 QI Fuli 2018-07-07 18:56 ` Dan Williams 0 siblings, 1 reply; 5+ messages in thread From: QI Fuli @ 2018-07-07 5:35 UTC (permalink / raw) To: linux-nvdimm Add a new unit test to test the following options of the monitor command. --dimm --bus --region --namespace --logfile --config-file Based-on-patch-by: Yasunori Goto <y-goto@jp.fujitsu.com> Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> --- v2 -> v3: - Add filter_obj instead of hard-coded values v1 -> v2: - Add init() - Add get_filter_dimm() to get the filter dimms by ndctl list command instead of hard-cording - Add sleep to call_notify() test/Makefile.am | 3 +- test/monitor.sh | 134 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100755 test/monitor.sh diff --git a/test/Makefile.am b/test/Makefile.am index cd451e9..8c76462 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -21,7 +21,8 @@ TESTS =\ btt-pad-compat.sh \ firmware-update.sh \ ack-shutdown-count-set \ - rescan-partitions.sh + rescan-partitions.sh \ + monitor.sh check_PROGRAMS =\ libndctl \ diff --git a/test/monitor.sh b/test/monitor.sh new file mode 100755 index 0000000..4a7d733 --- /dev/null +++ b/test/monitor.sh @@ -0,0 +1,134 @@ +#!/bin/bash -Ex + +# SPDX-License-Identifier: GPL-2.0 +# Copyright(c) 2018, FUJITSU LIMITED. All rights reserved. + +rc=77 +logfile="" +conf_file="" +filter_dimms="" +filter_obj="" +monitor_pid=65536 + +. ./common + +trap 'err $LINENO' ERR + +check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor service" + +start_monitor() +{ + logfile=$(mktemp) + $NDCTL monitor -l $logfile $1 & + monitor_pid=$! + truncate --size 0 $logfile #remove startup log +} + +get_filter_dimm() +{ + jlist=$($NDCTL list -D -b $NFIT_TEST_BUS0 $1) + filter_dimms=$(jq '.[]."dev"?, ."dev"?' <<<$jlist | sort | uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') +} + +call_notify() +{ + ./smart-notify $NFIT_TEST_BUS0 + sync; sleep 3 +} + +check_result() +{ + jlog=$(cat $logfile) + notify_dimms=$(jq ."dimm"."dev" <<<$jlog | sort | uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') + [[ $filter_dimms == $notify_dimms ]] +} + +stop_monitor() +{ + kill $monitor_pid + rm $logfile +} + +create_conf_file() +{ + conf_file=$(mktemp) + echo "dimm = $1" > $conf_file + cat $conf_file +} + +test_filter_dimm() +{ + filter_obj=$($NDCTL list -D -b $NFIT_TEST_BUS0 | jq -r .[0].dev) + get_filter_dimm "-d $filter_obj" + start_monitor "-d $filter_obj" + call_notify + check_result + stop_monitor +} + +test_filter_bus() +{ + get_filter_dimm + start_monitor "-b $NFIT_TEST_BUS0" + call_notify + check_result + stop_monitor +} + +test_filter_region() +{ + filter_obj=$($NDCTL list -R -b $NFIT_TEST_BUS0 | jq -r .[0].dev) + get_filter_dimm "-r $filter_obj" + start_monitor "-r $filter_obj" + call_notify + check_result + stop_monitor +} + +test_filter_namespace() +{ + filter_obj=$($NDCTL list -N -b $NFIT_TEST_BUS0 | jq -r .[0].dev) + [ -z $filter_obj ] && filter_obj="namespace1.0" + $NDCTL create-namespace -r $($NDCTL list -R -b $NFIT_TEST_BUS0 | jq -r .[0].dev) -n $filter_obj + get_filter_dimm "-n $filter_obj" + start_monitor "-n $filter_obj" + call_notify + check_result + stop_monitor + $NDCTL destroy-namespace $filter_obj -f +} + +test_conf_file() +{ + filter_obj=$($NDCTL list -D -b $NFIT_TEST_BUS0 | jq -r .[0].dev) + filter_dimms=$filter_obj + create_conf_file $filter_obj + start_monitor "-c $conf_file" + call_notify + check_result + stop_monitor + rm $conf_file +} + +do_tests() +{ + test_filter_dimm + test_filter_bus + test_filter_region + test_filter_namespace + test_conf_file +} + +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 +} + +modprobe nfit_test +rc=1 +init +do_tests +_cleanup +exit 0 -- 2.18.0 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor 2018-07-07 5:35 [ndctl PATCH v3] ndctl, test: add a new unit test for monitor QI Fuli @ 2018-07-07 18:56 ` Dan Williams 2018-07-09 11:38 ` Qi, Fuli 0 siblings, 1 reply; 5+ messages in thread From: Dan Williams @ 2018-07-07 18:56 UTC (permalink / raw) To: QI Fuli; +Cc: linux-nvdimm On Fri, Jul 6, 2018 at 10:35 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote: > Add a new unit test to test the following options of the monitor command. > --dimm > --bus > --region > --namespace > --logfile > --config-file > > Based-on-patch-by: Yasunori Goto <y-goto@jp.fujitsu.com> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > --- > v2 -> v3: > - Add filter_obj instead of hard-coded values > v1 -> v2: > - Add init() > - Add get_filter_dimm() to get the filter dimms by ndctl list command > instead of hard-cording > - Add sleep to call_notify() > > test/Makefile.am | 3 +- > test/monitor.sh | 134 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 136 insertions(+), 1 deletion(-) > create mode 100755 test/monitor.sh This test is still failing for me, here is the tail of the test/test-suite.log output: + sync + sleep 3 + check_result ++ cat /tmp/tmp.dT3TJ4l5IE + jlog='nmem0: no smart support no dimms to monitor' ++ jq .dimm.dev ++ sort ++ uniq ++ sed -e ':loop; N; $!b loop; s/\n/:/g' ++ sed 's/\"//g' parse error: Invalid literal at line 1, column 6 + notify_dimms= + [[ '' == '' ]] + stop_monitor + kill 39414 ./monitor.sh: line 48: kill: (39414) - No such process ++ err 48 +++ basename ./monitor.sh ++ echo test/monitor.sh: failed at line 48 test/monitor.sh: failed at line 48 ++ '[' -n '' ']' ++ exit 1 FAIL monitor.sh (exit status: 1) I notice errors around get_filter_dimm(). Why is that function needed, it seems redundant now that $filter_obj is being used? Hmm, if I just delete get_filter_dimm() I get this; ++ jq .dimm.dev ++ sort ++ uniq ++ sed -e ':loop; N; $!b loop; s/\n/:/g' ++ sed 's/\"//g' + notify_dimms=nmem3 + [[ '' == nmem3 ]] ++ err 34 +++ basename ./monitor.sh ++ echo test/monitor.sh: failed at line 34 test/monitor.sh: failed at line 34 ++ '[' -n '' ']' ++ exit 1 FAIL monitor.sh (exit status: 1) I assume this test is passing for you? Can you try running it inside a virtual machine that has a virtual NVDIMM so you can debug the collisions between the nfit_test.0 bus objects and the ACPI.NFIT ones? Here is my test environment where this unit test is failing: https://gist.github.com/djbw/144dc5ddaf5e935ad58bd66a702a5ea8 > > diff --git a/test/Makefile.am b/test/Makefile.am > index cd451e9..8c76462 100644 > --- a/test/Makefile.am > +++ b/test/Makefile.am > @@ -21,7 +21,8 @@ TESTS =\ > btt-pad-compat.sh \ > firmware-update.sh \ > ack-shutdown-count-set \ > - rescan-partitions.sh > + rescan-partitions.sh \ > + monitor.sh > > check_PROGRAMS =\ > libndctl \ > diff --git a/test/monitor.sh b/test/monitor.sh > new file mode 100755 > index 0000000..4a7d733 > --- /dev/null > +++ b/test/monitor.sh > @@ -0,0 +1,134 @@ > +#!/bin/bash -Ex > + > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright(c) 2018, FUJITSU LIMITED. All rights reserved. > + > +rc=77 > +logfile="" > +conf_file="" > +filter_dimms="" > +filter_obj="" > +monitor_pid=65536 > + > +. ./common > + > +trap 'err $LINENO' ERR > + > +check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor service" > + > +start_monitor() > +{ > + logfile=$(mktemp) > + $NDCTL monitor -l $logfile $1 & > + monitor_pid=$! > + truncate --size 0 $logfile #remove startup log > +} > + > +get_filter_dimm() > +{ > + jlist=$($NDCTL list -D -b $NFIT_TEST_BUS0 $1) > + filter_dimms=$(jq '.[]."dev"?, ."dev"?' <<<$jlist | sort | uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') > +} Can we just remove this? > + > +call_notify() > +{ > + ./smart-notify $NFIT_TEST_BUS0 > + sync; sleep 3 > +} > + > +check_result() > +{ > + jlog=$(cat $logfile) > + notify_dimms=$(jq ."dimm"."dev" <<<$jlog | sort | uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') Can you add a comment here about that sed script is trying to do? If it's just filtering json I'd rather it just jq directly. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor 2018-07-07 18:56 ` Dan Williams @ 2018-07-09 11:38 ` Qi, Fuli 2018-07-09 15:45 ` Dan Williams 0 siblings, 1 reply; 5+ messages in thread From: Qi, Fuli @ 2018-07-09 11:38 UTC (permalink / raw) To: 'Dan Williams'; +Cc: linux-nvdimm Hi Dan, Thanks for your comments. > -----Original Message----- > From: Dan Williams [mailto:dan.j.williams@intel.com] > Sent: Sunday, July 8, 2018 3:56 AM > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com> > Cc: linux-nvdimm <linux-nvdimm@lists.01.org> > Subject: Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor > > On Fri, Jul 6, 2018 at 10:35 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote: > > Add a new unit test to test the following options of the monitor command. > > --dimm > > --bus > > --region > > --namespace > > --logfile > > --config-file > > > > Based-on-patch-by: Yasunori Goto <y-goto@jp.fujitsu.com> > > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > > --- > > v2 -> v3: > > - Add filter_obj instead of hard-coded values > > v1 -> v2: > > - Add init() > > - Add get_filter_dimm() to get the filter dimms by ndctl list command > > instead of hard-cording > > - Add sleep to call_notify() > > > > test/Makefile.am | 3 +- > > test/monitor.sh | 134 > > +++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100755 > > test/monitor.sh > > This test is still failing for me, here is the tail of the test/test-suite.log output: > > + sync > + sleep 3 > + check_result > ++ cat /tmp/tmp.dT3TJ4l5IE > + jlog='nmem0: no smart support > My idea of [--dimm option] unit test is try to get the first dimm on bus nfit_test.0, then let monitor to run [-d] option with it, the same as: ndctl monitor -b nfit_test.0 -d nmem0 but in your environment, since the "nmem0" didn't support smart, then the monitor outputted the following error message and stopped. > no dimms to monitor' > ++ jq .dimm.dev > ++ sort > ++ uniq > ++ sed -e ':loop; N; $!b loop; s/\n/:/g' > ++ sed 's/\"//g' > parse error: Invalid literal at line 1, column 6 > + notify_dimms= > + [[ '' == '' ]] > + stop_monitor > + kill 39414 > ./monitor.sh: line 48: kill: (39414) - No such process > ++ err 48 > +++ basename ./monitor.sh > ++ echo test/monitor.sh: failed at line 48 > test/monitor.sh: failed at line 48 > ++ '[' -n '' ']' > ++ exit 1 > FAIL monitor.sh (exit status: 1) > > I notice errors around get_filter_dimm(). Why is that function needed, it seems > redundant now that $filter_obj is being used? > I am sorry that the "filter_dimm" is not a suitable name, maybe changing it to "monitor_dimm" will be better to understand. This function is used to get dimms to be monitored from "ndctl list -D -b nfit_test.0 $1". After "./smart-notify nfit_test.0", get "notify dimms" from output notifications. Then compare the "monitor dimms" with the "notify dimms", if same that means the unit test option works well. I will add the a filter to make sure that the "monitor dimms" support smart event in next version. > Hmm, if I just delete get_filter_dimm() I get this; > > ++ jq .dimm.dev > ++ sort > ++ uniq > ++ sed -e ':loop; N; $!b loop; s/\n/:/g' > ++ sed 's/\"//g' > + notify_dimms=nmem3 > + [[ '' == nmem3 ]] > ++ err 34 > +++ basename ./monitor.sh > ++ echo test/monitor.sh: failed at line 34 > test/monitor.sh: failed at line 34 > ++ '[' -n '' ']' > ++ exit 1 > FAIL monitor.sh (exit status: 1) > > I assume this test is passing for you? Can you try running it inside a virtual machine > that has a virtual NVDIMM so you can debug the collisions between the nfit_test.0 > bus objects and the ACPI.NFIT ones? > I passed this unit test on a virtual machine, which has a virtual NVDIMM. Here is the buses and dimms list of my test environment. $ sudo ndctl list -BD [ { "provider":"nfit_test.0", "dev":"ndbus1", "scrub_state":"idle", "dimms":[ { "dev":"nmem1", "id":"cdab-0a-07e0-feffffff", "handle":1, "phys_id":1 }, { "dev":"nmem3", "id":"cdab-0a-07e0-fefeffff", "handle":257, "phys_id":3 }, { "dev":"nmem0", "id":"cdab-0a-07e0-ffffffff", "handle":0, "phys_id":0 }, { "dev":"nmem2", "id":"cdab-0a-07e0-fffeffff", "handle":256, "phys_id":2 } ] }, { "provider":"e820", "dev":"ndbus0" }, { "provider":"nfit_test.1", "dev":"ndbus2", "scrub_state":"idle", "dimms":[ { "dev":"nmem5", "id":"cdab-0a-07e0-fefffeff", "handle":65537, "phys_id":0, "flag_failed_map":true }, { "dev":"nmem4", "id":"cdab-0a-07e0-fffffeff", "handle":65536, "phys_id":0, "flag_failed_save":true, "flag_failed_arm":true, "flag_failed_restore":true, "flag_failed_flush":true, "flag_smart_event":true } ] } ] > Here is my test environment where this unit test is failing: > > https://gist.github.com/djbw/144dc5ddaf5e935ad58bd66a702a5ea8 > > > > > diff --git a/test/Makefile.am b/test/Makefile.am index > > cd451e9..8c76462 100644 > > --- a/test/Makefile.am > > +++ b/test/Makefile.am > > @@ -21,7 +21,8 @@ TESTS =\ > > btt-pad-compat.sh \ > > firmware-update.sh \ > > ack-shutdown-count-set \ > > - rescan-partitions.sh > > + rescan-partitions.sh \ > > + monitor.sh > > > > check_PROGRAMS =\ > > libndctl \ > > diff --git a/test/monitor.sh b/test/monitor.sh new file mode 100755 > > index 0000000..4a7d733 > > --- /dev/null > > +++ b/test/monitor.sh > > @@ -0,0 +1,134 @@ > > +#!/bin/bash -Ex > > + > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright(c) 2018, FUJITSU LIMITED. All rights reserved. > > + > > +rc=77 > > +logfile="" > > +conf_file="" > > +filter_dimms="" > > +filter_obj="" > > +monitor_pid=65536 > > + > > +. ./common > > + > > +trap 'err $LINENO' ERR > > + > > +check_min_kver "4.15" || do_skip "kernel $KVER may not support monitor service" > > + > > +start_monitor() > > +{ > > + logfile=$(mktemp) > > + $NDCTL monitor -l $logfile $1 & > > + monitor_pid=$! > > + truncate --size 0 $logfile #remove startup log } > > + > > +get_filter_dimm() > > +{ > > + jlist=$($NDCTL list -D -b $NFIT_TEST_BUS0 $1) > > + filter_dimms=$(jq '.[]."dev"?, ."dev"?' <<<$jlist | sort | > > +uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') } > > Can we just remove this? As I mentioned above, I don't think so. > > > + > > +call_notify() > > +{ > > + ./smart-notify $NFIT_TEST_BUS0 > > + sync; sleep 3 > > +} > > + > > +check_result() > > +{ > > + jlog=$(cat $logfile) > > + notify_dimms=$(jq ."dimm"."dev" <<<$jlog | sort | uniq | sed > > +-e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') > > Can you add a comment here about that sed script is trying to do? If it's just filtering > json I'd rather it just jq directly. These sed scripts are used to format "monitor dimms" and "notify dimms". The first one is task to replace "\n" with ":", and the second one is used to remove ' " '. After these processes, the format will become "nmem0:nmem1:nmem2". _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor 2018-07-09 11:38 ` Qi, Fuli @ 2018-07-09 15:45 ` Dan Williams 2018-07-10 7:04 ` Qi, Fuli 0 siblings, 1 reply; 5+ messages in thread From: Dan Williams @ 2018-07-09 15:45 UTC (permalink / raw) To: Qi, Fuli; +Cc: linux-nvdimm On Mon, Jul 9, 2018 at 4:38 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote: > Hi Dan, > Thanks for your comments. > >> -----Original Message----- >> From: Dan Williams [mailto:dan.j.williams@intel.com] >> Sent: Sunday, July 8, 2018 3:56 AM >> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com> >> Cc: linux-nvdimm <linux-nvdimm@lists.01.org> >> Subject: Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor >> >> On Fri, Jul 6, 2018 at 10:35 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote: >> > Add a new unit test to test the following options of the monitor command. >> > --dimm >> > --bus >> > --region >> > --namespace >> > --logfile >> > --config-file >> > >> > Based-on-patch-by: Yasunori Goto <y-goto@jp.fujitsu.com> >> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> >> > --- >> > v2 -> v3: >> > - Add filter_obj instead of hard-coded values >> > v1 -> v2: >> > - Add init() >> > - Add get_filter_dimm() to get the filter dimms by ndctl list command >> > instead of hard-cording >> > - Add sleep to call_notify() >> > >> > test/Makefile.am | 3 +- >> > test/monitor.sh | 134 >> > +++++++++++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100755 >> > test/monitor.sh >> >> This test is still failing for me, here is the tail of the test/test-suite.log output: >> >> + sync >> + sleep 3 >> + check_result >> ++ cat /tmp/tmp.dT3TJ4l5IE >> + jlog='nmem0: no smart support >> > > My idea of [--dimm option] unit test is try to get the first dimm on bus nfit_test.0, then let > monitor to run [-d] option with it, the same as: > ndctl monitor -b nfit_test.0 -d nmem0 > but in your environment, since the "nmem0" didn't support smart, then the monitor outputted the following error message and stopped. Right, the test is fragile if it tries to guess nmemX device names. > >> no dimms to monitor' > >> ++ jq .dimm.dev >> ++ sort >> ++ uniq >> ++ sed -e ':loop; N; $!b loop; s/\n/:/g' >> ++ sed 's/\"//g' >> parse error: Invalid literal at line 1, column 6 >> + notify_dimms= >> + [[ '' == '' ]] >> + stop_monitor >> + kill 39414 >> ./monitor.sh: line 48: kill: (39414) - No such process >> ++ err 48 >> +++ basename ./monitor.sh >> ++ echo test/monitor.sh: failed at line 48 >> test/monitor.sh: failed at line 48 >> ++ '[' -n '' ']' >> ++ exit 1 >> FAIL monitor.sh (exit status: 1) >> >> I notice errors around get_filter_dimm(). Why is that function needed, it seems >> redundant now that $filter_obj is being used? >> > > I am sorry that the "filter_dimm" is not a suitable name, maybe changing it to "monitor_dimm" will be better to understand. > This function is used to get dimms to be monitored from "ndctl list -D -b nfit_test.0 $1". > After "./smart-notify nfit_test.0", get "notify dimms" from output notifications. > Then compare the "monitor dimms" with the "notify dimms", if same that means the unit test option works well. > I will add the a filter to make sure that the "monitor dimms" support smart event in next version. I'm confused, how does checking for smart event support get around the fact that the test is looking for the wrong DIMM names in the first place? > >> Hmm, if I just delete get_filter_dimm() I get this; >> >> ++ jq .dimm.dev >> ++ sort >> ++ uniq >> ++ sed -e ':loop; N; $!b loop; s/\n/:/g' >> ++ sed 's/\"//g' >> + notify_dimms=nmem3 >> + [[ '' == nmem3 ]] >> ++ err 34 >> +++ basename ./monitor.sh >> ++ echo test/monitor.sh: failed at line 34 >> test/monitor.sh: failed at line 34 >> ++ '[' -n '' ']' >> ++ exit 1 >> FAIL monitor.sh (exit status: 1) >> >> I assume this test is passing for you? Can you try running it inside a virtual machine >> that has a virtual NVDIMM so you can debug the collisions between the nfit_test.0 >> bus objects and the ACPI.NFIT ones? >> > [..] >> > +{ >> > + jlist=$($NDCTL list -D -b $NFIT_TEST_BUS0 $1) >> > + filter_dimms=$(jq '.[]."dev"?, ."dev"?' <<<$jlist | sort | >> > +uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') } >> >> Can we just remove this? > > As I mentioned above, I don't think so. Again, I don't see how you can achieve the verification you want without first discovering the names of the possible DIMMs on nfit_test.0. There is no reliable way that you can hard code the DIMM names. > >> >> > + >> > +call_notify() >> > +{ >> > + ./smart-notify $NFIT_TEST_BUS0 >> > + sync; sleep 3 >> > +} >> > + >> > +check_result() >> > +{ >> > + jlog=$(cat $logfile) >> > + notify_dimms=$(jq ."dimm"."dev" <<<$jlog | sort | uniq | sed >> > +-e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') >> >> Can you add a comment here about that sed script is trying to do? If it's just filtering >> json I'd rather it just jq directly. > > These sed scripts are used to format "monitor dimms" and "notify dimms". > The first one is task to replace "\n" with ":", and the second one is used to remove ' " '. > After these processes, the format will become "nmem0:nmem1:nmem2". That shell script just seems too dense for what it is trying to do. How about something like: # ndctl list -D | jq -r .[].dev | xargs That will take the output of 'list' and produce a space separated list. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor 2018-07-09 15:45 ` Dan Williams @ 2018-07-10 7:04 ` Qi, Fuli 0 siblings, 0 replies; 5+ messages in thread From: Qi, Fuli @ 2018-07-10 7:04 UTC (permalink / raw) To: 'Dan Williams'; +Cc: linux-nvdimm > -----Original Message----- > From: Dan Williams [mailto:dan.j.williams@intel.com] > Sent: Tuesday, July 10, 2018 12:46 AM > To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com> > Cc: linux-nvdimm <linux-nvdimm@lists.01.org> > Subject: Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor > > On Mon, Jul 9, 2018 at 4:38 AM, Qi, Fuli <qi.fuli@jp.fujitsu.com> wrote: > > Hi Dan, > > Thanks for your comments. > > > >> -----Original Message----- > >> From: Dan Williams [mailto:dan.j.williams@intel.com] > >> Sent: Sunday, July 8, 2018 3:56 AM > >> To: Qi, Fuli/斉 福利 <qi.fuli@jp.fujitsu.com> > >> Cc: linux-nvdimm <linux-nvdimm@lists.01.org> > >> Subject: Re: [ndctl PATCH v3] ndctl, test: add a new unit test for > >> monitor > >> > >> On Fri, Jul 6, 2018 at 10:35 PM, QI Fuli <qi.fuli@jp.fujitsu.com> wrote: > >> > Add a new unit test to test the following options of the monitor command. > >> > --dimm > >> > --bus > >> > --region > >> > --namespace > >> > --logfile > >> > --config-file > >> > > >> > Based-on-patch-by: Yasunori Goto <y-goto@jp.fujitsu.com> > >> > Signed-off-by: QI Fuli <qi.fuli@jp.fujitsu.com> > >> > --- > >> > v2 -> v3: > >> > - Add filter_obj instead of hard-coded values > >> > v1 -> v2: > >> > - Add init() > >> > - Add get_filter_dimm() to get the filter dimms by ndctl list command > >> > instead of hard-cording > >> > - Add sleep to call_notify() > >> > > >> > test/Makefile.am | 3 +- > >> > test/monitor.sh | 134 > >> > +++++++++++++++++++++++++++++++++++++++++++++++ > >> > 2 files changed, 136 insertions(+), 1 deletion(-) create mode > >> > 100755 test/monitor.sh > >> > >> This test is still failing for me, here is the tail of the test/test-suite.log > output: > >> > >> + sync > >> + sleep 3 > >> + check_result > >> ++ cat /tmp/tmp.dT3TJ4l5IE > >> + jlog='nmem0: no smart support > >> > > > > My idea of [--dimm option] unit test is try to get the first dimm on > > bus nfit_test.0, then let monitor to run [-d] option with it, the same as: > > ndctl monitor -b nfit_test.0 -d nmem0 but in your environment, > > since the "nmem0" didn't support smart, then the monitor outputted the following > error message and stopped. > > Right, the test is fragile if it tries to guess nmemX device names. > > > > >> no dimms to monitor' > > > >> ++ jq .dimm.dev > >> ++ sort > >> ++ uniq > >> ++ sed -e ':loop; N; $!b loop; s/\n/:/g' > >> ++ sed 's/\"//g' > >> parse error: Invalid literal at line 1, column 6 > >> + notify_dimms= > >> + [[ '' == '' ]] > >> + stop_monitor > >> + kill 39414 > >> ./monitor.sh: line 48: kill: (39414) - No such process > >> ++ err 48 > >> +++ basename ./monitor.sh > >> ++ echo test/monitor.sh: failed at line 48 > >> test/monitor.sh: failed at line 48 > >> ++ '[' -n '' ']' > >> ++ exit 1 > >> FAIL monitor.sh (exit status: 1) > >> > >> I notice errors around get_filter_dimm(). Why is that function > >> needed, it seems redundant now that $filter_obj is being used? > >> > > > > I am sorry that the "filter_dimm" is not a suitable name, maybe changing it to > "monitor_dimm" will be better to understand. > > This function is used to get dimms to be monitored from "ndctl list -D -b nfit_test.0 > $1". > > After "./smart-notify nfit_test.0", get "notify dimms" from output notifications. > > Then compare the "monitor dimms" with the "notify dimms", if same that means the > unit test option works well. > > I will add the a filter to make sure that the "monitor dimms" support smart event > in next version. > > I'm confused, how does checking for smart event support get around the fact that > the test is looking for the wrong DIMM names in the first place? > I want to add a helper function "list-smart-dimm", which can list and filter all of the smart supported dimms. The "list-smart-dimm" could have the [-d] [-b] [-n] [-r] options the same as list. For example to test the [-r] option of monitor: First, we can get the "monitor_dimms = list-smart-dimm -b nfit_test.0 -r regionX". If "monitor_dimms" is NULL, then get the "monitor_dimms" form "list-smart-dimm -b nfit_test.1 -r regionX". Next, start monitor "ndctl monitor -b nfit_test.0 -r regionX". We can get the "notify_dimms" logged by monitor after do the "smart-notify nfit_test.0(1)". Then check the result, if "monitor_dimms" is the same as "notify_dimms", it means the monitor works well. > > > >> Hmm, if I just delete get_filter_dimm() I get this; > >> > >> ++ jq .dimm.dev > >> ++ sort > >> ++ uniq > >> ++ sed -e ':loop; N; $!b loop; s/\n/:/g' > >> ++ sed 's/\"//g' > >> + notify_dimms=nmem3 > >> + [[ '' == nmem3 ]] > >> ++ err 34 > >> +++ basename ./monitor.sh > >> ++ echo test/monitor.sh: failed at line 34 > >> test/monitor.sh: failed at line 34 > >> ++ '[' -n '' ']' > >> ++ exit 1 > >> FAIL monitor.sh (exit status: 1) > >> > >> I assume this test is passing for you? Can you try running it inside > >> a virtual machine that has a virtual NVDIMM so you can debug the > >> collisions between the nfit_test.0 bus objects and the ACPI.NFIT ones? > >> > > > [..] > > >> > +{ > >> > + jlist=$($NDCTL list -D -b $NFIT_TEST_BUS0 $1) > >> > + filter_dimms=$(jq '.[]."dev"?, ."dev"?' <<<$jlist | sort | > >> > +uniq | sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') } > >> > >> Can we just remove this? > > > > As I mentioned above, I don't think so. > > Again, I don't see how you can achieve the verification you want without first > discovering the names of the possible DIMMs on nfit_test.0. There is no reliable > way that you can hard code the DIMM names. > > > > >> > >> > + > >> > +call_notify() > >> > +{ > >> > + ./smart-notify $NFIT_TEST_BUS0 > >> > + sync; sleep 3 > >> > +} > >> > + > >> > +check_result() > >> > +{ > >> > + jlog=$(cat $logfile) > >> > + notify_dimms=$(jq ."dimm"."dev" <<<$jlog | sort | uniq | > >> > +sed -e ':loop; N; $!b loop; s/\n/:/g' | sed 's/\"//g') > >> > >> Can you add a comment here about that sed script is trying to do? If > >> it's just filtering json I'd rather it just jq directly. > > > > These sed scripts are used to format "monitor dimms" and "notify dimms". > > The first one is task to replace "\n" with ":", and the second one is used to remove > ' " '. > > After these processes, the format will become "nmem0:nmem1:nmem2". > > That shell script just seems too dense for what it is trying to do. > How about something like: > > # ndctl list -D | jq -r .[].dev | xargs > > That will take the output of 'list' and produce a space separated list. > Yes, I will replace the sed scripts with xargs. Thank you very much. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-10 7:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-07 5:35 [ndctl PATCH v3] ndctl, test: add a new unit test for monitor QI Fuli 2018-07-07 18:56 ` Dan Williams 2018-07-09 11:38 ` Qi, Fuli 2018-07-09 15:45 ` Dan Williams 2018-07-10 7:04 ` Qi, Fuli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).