[ndctl PATCH v3] ndctl, test: add a new unit test for monitor

[ndctl PATCH v3] ndctl, test: add a new unit test for monitor

[QI Fuli]

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

Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor

[Dan Williams]

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

RE: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor

[Qi, Fuli]

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

Re: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor

[Dan Williams]

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

RE: [ndctl PATCH v3] ndctl, test: add a new unit test for monitor

[Qi, Fuli]

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