linux-kernel - Re: [PATCH v4 37/37] selftests: ftrace: Add inter-event hist triggers testcases

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 Oct 2017 23:29:04 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Tom Zanussi <tom.zanussi@...ux.intel.com>
Cc:     rostedt@...dmis.org, tglx@...utronix.de, mhiramat@...nel.org,
        namhyung@...nel.org, vedang.patel@...el.com, bigeasy@...utronix.de,
        joel.opensrc@...il.com, joelaf@...gle.com,
        mathieu.desnoyers@...icios.com, baohong.liu@...el.com,
        rajvi.jingar@...el.com, julia@...com, linux-kernel@...r.kernel.org,
        linux-rt-users@...r.kernel.org
Subject: Re: [PATCH v4 37/37] selftests: ftrace: Add inter-event hist
 triggers testcases

On Mon, 30 Oct 2017 15:52:19 -0500
Tom Zanussi <tom.zanussi@...ux.intel.com> wrote:

> From: Rajvi Jingar <rajvi.jingar@...el.com>
> 
> This adds inter-event hist triggers testcases which covers following:
>  - create/remove synthetic event
>  - disable histogram for synthetic event
>  - extended error support
>  - field variable support
>  - histogram variables
>  - histogram trigger onmatch action
>  - histogram trigger onmax action
>  - histogram trigger onmatch-onmax action
>  - simple expression support
>  - combined histogram

Thank you for adding testcases :)
That helps me to understand how to use it.
I have some comments, see below;


[..]
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-extended-error-support.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-extended-error-support.tc
> new file mode 100644
> index 0000000..41dbf4c
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-extended-error-support.tc
> @@ -0,0 +1,39 @@
> +#!/bin/sh
> +# description: event trigger - test extended error support
> +
> +
> +do_reset() {
> +    reset_trigger
> +    echo > set_event
> +    clear_trace
> +}
> +
> +fail() { #msg
> +    do_reset
> +    echo $1
> +    exit $FAIL
> +}
> +
> +if [ ! -f set_event ]; then
> +    echo "event tracing is not supported"
> +    exit_unsupported
> +fi
> +
> +if [ ! -f synthetic_events ]; then
> +    echo "synthetic event is not supported"
> +    exit_unsupported
> +fi
> +
> +reset_tracer
> +do_reset
> +
> +echo "Test extended error support"
> +echo 'hist:keys=pid:ts0=$common_timestamp.usecs if comm=="ping"' >> events/sched/sched_wakeup/trigger
> +echo 'hist:keys=pid:ts0=$common_timestamp.usecs if comm=="ping"' >> events/sched/sched_wakeup/trigger &&

What is the last "&&"? Would you expect that the second echo is fail?

> +if ! grep "ERROR:" events/sched/sched_wakeup/hist; then

Would you like to show the grep result? or you can use -q option for grep command.

[...]
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-createremove.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-createremove.tc
> new file mode 100644
> index 0000000..c73e491
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-createremove.tc
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +# description: event trigger - test synthetic event create remove
> +do_reset() {
> +    reset_trigger
> +    echo > set_event
> +    clear_trace
> +}
> +
> +fail() { #msg
> +    do_reset
> +    echo $1
> +    exit $FAIL
> +}
> +
> +if [ ! -f set_event ]; then
> +    echo "event tracing is not supported"
> +    exit_unsupported
> +fi
> +
> +if [ ! -f synthetic_events ]; then
> +    echo "synthetic event is not supported"
> +    exit_unsupported
> +fi
> +
> +clear_synthetic_events
> +reset_tracer
> +do_reset
> +
> +echo "Test create synthetic event"
> +
> +echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' > synthetic_events
> +if [ ! -d events/synthetic/wakeup_latency ]; then
> +    fail "Failed to create wakeup_latency synthetic event"
> +fi
> +
> +reset_trigger
> +
> +echo "Test remove synthetic event"
> +echo '!wakeup_latency  u64 lat; pid_t pid; char[16] comm' > synthetic_events

both char[16] and char comm[16] are acceptable?
But I would like to see same style in creating and removing.

> +if [ -d events/synthetic/wakeup_latency ]; then
> +    fail "Failed to delete wakeup_latency synthetic event"
> +fi

There seems no expected failure test cases. Could you add such test?
like as echoing events with invalid format etc.


<off topic>
BTW, $FAIL is not for expressing failure, it is a value
for $SIG_RESULT. But it seems many test cases use it for return code.

Note that any failure in this script treated as error and exit soon. 
Which means caller can get any return code from this script, because
the return code depends on executed command. So, any NON-ZERO code
can be treated as FAILURE code.

"exit $FAIL" is OK, but just making myself review easier, I'll
introduce exit_pass()/exit_fail().
<off topic>

Thank you,

-- 
Masami Hiramatsu <mhiramat@...nel.org>