RE: [ndctl PATCH v11 5/5] ndctl, test: add a new unit test for monitor
> 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 000..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
Re: [ndctl PATCH v11 5/5] ndctl, test: add a new unit test for monitor
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 000..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))
>
> > + 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
> > +
Re: [ndctl PATCH v11 5/5] ndctl, test: add a new unit test for monitor
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 000..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...
> + 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()
> +{
> +
[ndctl PATCH v11 5/5] ndctl, test: add a new unit test for monitor
Add a new unit test to test all options of the monitor command.
Based-on-patch-by: Yasunori Goto
Signed-off-by: QI Fuli
---
.gitignore | 1 +
test/Makefile.am | 14 +++-
test/list-smart-dimm.c | 117 +++
test/monitor.sh| 177 +
4 files changed, 307 insertions(+), 2 deletions(-)
create mode 100644 test/list-smart-dimm.c
create mode 100755 test/monitor.sh
diff --git a/.gitignore b/.gitignore
index 1016b3b..0baace4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -56,3 +56,4 @@ test/smart-notify
test/fio.job
test/local-write-0-verify.state
test/ack-shutdown-count-set
+test/list-smart-dimm
diff --git a/test/Makefile.am b/test/Makefile.am
index cd451e9..8c55056 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 \
@@ -34,7 +35,8 @@ check_PROGRAMS =\
smart-listen \
hugetlb \
daxdev-errors \
- ack-shutdown-count-set
+ ack-shutdown-count-set \
+ list-smart-dimm
if ENABLE_DESTRUCTIVE
TESTS +=\
@@ -151,3 +153,11 @@ multi_pmem_LDADD = \
$(UUID_LIBS) \
$(KMOD_LIBS) \
../libutil.a
+
+list_smart_dimm_SOURCES = \
+ list-smart-dimm.c \
+ ../util/json.c
+list_smart_dimm_LDADD = \
+ $(LIBNDCTL_LIB) \
+ $(JSON_LIBS) \
+ ../libutil.a
diff --git a/test/list-smart-dimm.c b/test/list-smart-dimm.c
new file mode 100644
index 000..c9982d5
--- /dev/null
+++ b/test/list-smart-dimm.c
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2018, FUJITSU LIMITED. All rights reserved. */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct util_filter_params param;
+static int did_fail;
+static int jflag = JSON_C_TO_STRING_PRETTY;
+
+#define fail(fmt, ...) \
+do { \
+ did_fail = 1; \
+ fprintf(stderr, "ndctl-%s:%s:%d: " fmt, \
+ VERSION, __func__, __LINE__, ##__VA_ARGS__); \
+} while (0)
+
+static bool filter_region(struct ndctl_region *region,
+ struct util_filter_ctx *ctx)
+{
+ return true;
+}
+
+static void filter_dimm(struct ndctl_dimm *dimm, struct util_filter_ctx *ctx)
+{
+ struct list_filter_arg *lfa = ctx->list;
+ struct json_object *jdimm;
+
+ if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART))
+ return;
+ if (!ndctl_dimm_is_cmd_supported(dimm, ND_CMD_SMART_THRESHOLD))
+ return;
+ if (!ndctl_dimm_is_flag_supported(dimm, ND_SMART_ALARM_VALID))
+ return;
+
+ if (!lfa->jdimms) {
+ lfa->jdimms = json_object_new_array();
+ if (!lfa->jdimms) {
+ fail("\n");
+ return;
+ }
+ }
+
+ jdimm = util_dimm_to_json(dimm, lfa->flags);
+ if (!jdimm) {
+ fail("\n");
+ return;
+ }
+
+ json_object_array_add(lfa->jdimms, jdimm);
+}
+
+static bool filter_bus(struct ndctl_bus *bus, struct util_filter_ctx *ctx)
+{
+ return true;
+}
+
+static int list_display(struct list_filter_arg *lfa)
+{
+ struct json_object *jdimms = lfa->jdimms;
+
+ if (jdimms)
+ util_display_json_array(stdout, jdimms, jflag);
+ return 0;
+}
+
+int main(int argc, const char *argv[])
+{
+ struct ndctl_ctx *ctx;
+ int i, rc;
+ const struct option options[] = {
+ OPT_STRING('b', "bus", , "bus-id", "filter by bus"),
+ OPT_STRING('r', "region", , "region-id",
+ "filter by region"),
+ OPT_STRING('d', "dimm", , "dimm-id",
+ "filter by dimm"),
+ OPT_STRING('n', "namespace", , "namespace-id",
+ "filter by namespace id"),
+ OPT_END(),
+ };
+ const char * const u[] = {
+ "list-smart-dimm []",
+ NULL
+ };
+ struct util_filter_ctx fctx = { 0 };
+ struct list_filter_arg lfa = { 0 };
+
+ rc = ndctl_new();
+ if (rc < 0)
+ return EXIT_FAILURE;
+argc = parse_options(argc, argv, options, u, 0);
+ for (i = 0; i < argc; i++)
+ error("unknown parameter \"%s\"\n", argv[i]);
+ if (argc)
+ usage_with_options(u, options);
+
+ fctx.filter_bus = filter_bus;
+ fctx.filter_dimm = filter_dimm;
+ fctx.filter_region = filter_region;
+ fctx.filter_namespace = NULL;
+ fctx.list =
+ lfa.flags = 0;
+
+ rc = util_filter_walk(ctx, , );
+ if (rc)
+ return rc;
+
+ if (list_display() ||
