On Mon, Jul 9, 2018 at 4:38 AM, Qi, Fuli <qi.f...@jp.fujitsu.com> wrote:
> Hi Dan,
> Thanks for your comments.
>
>> -----Original Message-----
>> From: Dan Williams [mailto:dan.j.willi...@intel.com]
>> Sent: Sunday, July 8, 2018 3:56 AM
>> To: Qi, Fuli/斉 福利 <qi.f...@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.f...@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-g...@jp.fujitsu.com>
>> > Signed-off-by: QI Fuli <qi.f...@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
