> -----Original Message-----
> From: Dan Williams [mailto:[email protected]]
> Sent: Tuesday, July 10, 2018 12:46 AM
> To: Qi, Fuli/斉 福利 <[email protected]>
> Cc: linux-nvdimm <[email protected]>
> 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 <[email protected]> wrote:
> > Hi Dan,
> > Thanks for your comments.
> >
> >> -----Original Message-----
> >> From: Dan Williams [mailto:[email protected]]
> >> Sent: Sunday, July 8, 2018 3:56 AM
> >> To: Qi, Fuli/斉 福利 <[email protected]>
> >> Cc: linux-nvdimm <[email protected]>
> >> 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 <[email protected]> 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 <[email protected]>
> >> > Signed-off-by: QI Fuli <[email protected]>
> >> > ---
> >> > 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
[email protected]
https://lists.01.org/mailman/listinfo/linux-nvdimm
