Hi Masami,
> -----Original Message-----
> From: [email protected] [mailto:linux-rt-users-
> [email protected]] On Behalf Of Masami Hiramatsu
> Sent: Tuesday, October 31, 2017 7:29 AM
> To: Tom Zanussi <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Patel, Vedang <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; Liu, Baohong <[email protected]>;
> Jingar, Rajvi <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> 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 <[email protected]> wrote:
>
> > From: Rajvi Jingar <[email protected]>
> >
> > 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-ex
> > tended-error-support.tc
> > b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-ex
> > tended-error-support.tc
> > new file mode 100644
> > index 0000000..41dbf4c
> > --- /dev/null
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigge
> > +++ r-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?
>
Yes, The the second echo is expected to fail on trying to add same
trigger(mainly same variable name).
I'll change last "&&" to redirect error/output to /dev/null instead.
> > +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.
>
No, I don't need to show grep results. Will add -q option and will do so for
all the places with similar case.
> [...]
> > diff --git
> > a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-sy
> > nthetic-event-createremove.tc
> > b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-sy
> > nthetic-event-createremove.tc
> > new file mode 100644
> > index 0000000..c73e491
> > --- /dev/null
> > +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigge
> > +++ r-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.
>
Will change to 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.
>
Sure, will add a case trying to add an event with wrong format.
>
> <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 <[email protected]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the
> body of a message to [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
