Hi,
Here are some patches to update ftracetest to fix some issues. - [1/3] Fix coloring of XFAIL - [2/3] Fix a testcase not to expect just one event entry - [3/3] Do not use built-in echo because the behavior is different on dash and bash. (Thanks Liu for reporting!)
Thank you,
---
Masami Hiramatsu (3): selftests/ftrace: Make XFAIL green color selftests/ftrace: Pick only the first kprobe event to test selftests/ftrace: Use /bin/echo instead of built-in echo
tools/testing/selftests/ftrace/ftracetest | 2 +- tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 6 files changed, 12 insertions(+), 5 deletions(-)
-- Masami Hiramatsu (Linaro) mhiramat@kernel.org
Since XFAIL (Expected Failure) is expected to fail the test, which means that test case works as we expected. IOW, XFAIL is same as PASS. So make it green.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- tools/testing/selftests/ftrace/ftracetest | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/ftracetest b/tools/testing/selftests/ftrace/ftracetest index 063ecb290a5a..72e837d0dfc1 100755 --- a/tools/testing/selftests/ftrace/ftracetest +++ b/tools/testing/selftests/ftrace/ftracetest @@ -273,7 +273,7 @@ eval_result() { # sigval return $UNSUPPORTED_RESULT # depends on use case ;; $XFAIL) - prlog " [${color_red}XFAIL${color_reset}]" + prlog " [${color_green}XFAIL${color_reset}]" XFAILED_CASES="$XFAILED_CASES $CASENO" return 0 ;;
On Fri, 1 May 2020 22:37:41 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since XFAIL (Expected Failure) is expected to fail the test, which means that test case works as we expected. IOW, XFAIL is same as PASS. So make it green.
THANK YOU!!!! That's been annoying me for some time ;-)
BIG Acked-by: Steven Rostedt (VMware) rostedt@goodmis.org
-- Steve
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
tools/testing/selftests/ftrace/ftracetest | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/ftracetest b/tools/testing/selftests/ftrace/ftracetest index 063ecb290a5a..72e837d0dfc1 100755 --- a/tools/testing/selftests/ftrace/ftracetest +++ b/tools/testing/selftests/ftrace/ftracetest @@ -273,7 +273,7 @@ eval_result() { # sigval return $UNSUPPORTED_RESULT # depends on use case ;; $XFAIL)
prlog " [${color_red}XFAIL${color_reset}]"
prlog " [${color_green}XFAIL${color_reset}]" XFAILED_CASES="$XFAILED_CASES $CASENO" return 0
Since the kprobe/kprobe_args_type.tc reads out all event logs from the trace buffer, the test can fail if there is another fork event happens. Use head command to pick only the first kprobe event from the trace buffer to test the argument types.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- .../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable
: "Confirm the arguments is recorded in given types correctly" - ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` + ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
On Fri, 1 May 2020 22:37:51 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the kprobe/kprobe_args_type.tc reads out all event logs from the trace buffer, the test can fail if there is another fork event happens. Use head command to pick only the first kprobe event from the trace buffer to test the argument types.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
.../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable : "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
I think I've manually added this exact change to my tests to keep it from failing.
Reviewed-by: Steven Rostedt (VMware) rostedt@goodmis.org
-- Steve
On 5/1/20 8:17 AM, Steven Rostedt wrote:
On Fri, 1 May 2020 22:37:51 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the kprobe/kprobe_args_type.tc reads out all event logs from the trace buffer, the test can fail if there is another fork event happens. Use head command to pick only the first kprobe event from the trace buffer to test the argument types.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
.../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable : "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
I think I've manually added this exact change to my tests to keep it from failing.
Reviewed-by: Steven Rostedt (VMware) rostedt@goodmis.org
Does this conflict with:
Author: Xiao Yang yangx.jy@cn.fujitsu.com Date: Tue Apr 7 14:34:19 2020 +0800
selftests/ftrace: Check the first record for kprobe_args_type.tc
https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
I went into mainline yesterday in my rc4 pull request.
Exact change it appears.
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable
: "Confirm the arguments is recorded in given types correctly" - ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` + ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
thanks, -- Shuah
On Fri, 1 May 2020 09:38:59 -0600 shuah shuah@kernel.org wrote:
On 5/1/20 8:17 AM, Steven Rostedt wrote:
On Fri, 1 May 2020 22:37:51 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the kprobe/kprobe_args_type.tc reads out all event logs from the trace buffer, the test can fail if there is another fork event happens. Use head command to pick only the first kprobe event from the trace buffer to test the argument types.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
.../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable : "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
I think I've manually added this exact change to my tests to keep it from failing.
Reviewed-by: Steven Rostedt (VMware) rostedt@goodmis.org
Does this conflict with:
Author: Xiao Yang yangx.jy@cn.fujitsu.com Date: Tue Apr 7 14:34:19 2020 +0800
selftests/ftrace: Check the first record for kprobe_args_type.tchttps://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
I went into mainline yesterday in my rc4 pull request.
Exact change it appears.
Ah, then I guess we don't need this patch ;-)
-- Steve
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable
: "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*)
arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*)
arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"thanks, -- Shuah
On 5/1/20 10:20 AM, Steven Rostedt wrote:
On Fri, 1 May 2020 09:38:59 -0600 shuah shuah@kernel.org wrote:
On 5/1/20 8:17 AM, Steven Rostedt wrote:
On Fri, 1 May 2020 22:37:51 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the kprobe/kprobe_args_type.tc reads out all event logs from the trace buffer, the test can fail if there is another fork event happens. Use head command to pick only the first kprobe event from the trace buffer to test the argument types.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
.../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable : "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width : "Clear event for next loop"
I think I've manually added this exact change to my tests to keep it from failing.
Reviewed-by: Steven Rostedt (VMware) rostedt@goodmis.org
Does this conflict with:
Author: Xiao Yang yangx.jy@cn.fujitsu.com Date: Tue Apr 7 14:34:19 2020 +0800
selftests/ftrace: Check the first record for kprobe_args_type.tchttps://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
I went into mainline yesterday in my rc4 pull request.
Exact change it appears.
Ah, then I guess we don't need this patch ;-)
I took the first one. Will wait for patch 3 to finalize.
thanks, -- Shuah
On Fri, 1 May 2020 09:38:59 -0600 shuah shuah@kernel.org wrote:
On 5/1/20 8:17 AM, Steven Rostedt wrote:
On Fri, 1 May 2020 22:37:51 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the kprobe/kprobe_args_type.tc reads out all event logs from the trace buffer, the test can fail if there is another fork event happens. Use head command to pick only the first kprobe event from the trace buffer to test the argument types.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
.../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable : "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*) arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
I think I've manually added this exact change to my tests to keep it from failing.
Reviewed-by: Steven Rostedt (VMware) rostedt@goodmis.org
Does this conflict with:
Author: Xiao Yang yangx.jy@cn.fujitsu.com Date: Tue Apr 7 14:34:19 2020 +0800
selftests/ftrace: Check the first record for kprobe_args_type.tchttps://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co...
I went into mainline yesterday in my rc4 pull request.
Exact change it appears.
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc index 1bcb67dcae26..81490ecaaa92 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_args_type.tc @@ -38,7 +38,7 @@ for width in 64 32 16 8; do echo 0 > events/kprobes/testprobe/enable
: "Confirm the arguments is recorded in given types correctly"
- ARGS=`grep "testprobe" trace | sed -e 's/.* arg1=(.*) arg2=(.*)
arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'`
- ARGS=`grep "testprobe" trace | head -n 1 | sed -e 's/.* arg1=(.*)
arg2=(.*) arg3=(.*) arg4=(.*)/\1 \2 \3 \4/'` check_types $ARGS $width
: "Clear event for next loop"
Oops, yes, please drop this patch.
Thank you,
thanks, -- Shuah
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yiding yidingx.liu@intel.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E"
clear_trace() { # reset trace output echo > trace diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi
echo "Test histogram trace_marker tigger"
-echo 'hist:keys=common_pid' > events/ftrace/print/trigger +echo 'hist:keys=ip' > events/ftrace/print/trigger for i in `seq 1 10` ; do echo "hello" > trace_marker; done grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \ fail "hist trigger did not trigger correct times on trace_marker" diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc index 18b4d1c2807e..c1625d945f4d 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc @@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger + +# We have to use the built-in echo here because waking up pid must be same +# as echoing pid. +alias echo=echo sleep 1 echo "hello" > trace_marker
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc index dd262d6d0db6..23e52c8d71de 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc @@ -36,8 +36,8 @@ fi echo "Test histogram trace_marker to trace_marker latency histogram trigger"
echo 'latency u64 lat' > synthetic_events -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger echo -n "start" > trace_marker echo -n "end" > trace_marker
On Fri, 1 May 2020 22:38:00 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yiding yidingx.liu@intel.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E" clear_trace() { # reset trace output echo > trace diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi echo "Test histogram trace_marker tigger" -echo 'hist:keys=common_pid' > events/ftrace/print/trigger +echo 'hist:keys=ip' > events/ftrace/print/trigger
This is doing more than just changing the echo being used. It's changing the test being done.
for i in `seq 1 10` ; do echo "hello" > trace_marker; done grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \ fail "hist trigger did not trigger correct times on trace_marker" diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc index 18b4d1c2807e..c1625d945f4d 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc @@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
+# We have to use the built-in echo here because waking up pid must be same +# as echoing pid. +alias echo=echo sleep 1 echo "hello" > trace_marker diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc index dd262d6d0db6..23e52c8d71de 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc @@ -36,8 +36,8 @@ fi echo "Test histogram trace_marker to trace_marker latency histogram trigger" echo 'latency u64 lat' > synthetic_events -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
This too. And it's not explained in the change log why. In fact, these changes look like they belong in a separate patch.
-- Steve
echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger echo -n "start" > trace_marker echo -n "end" > trace_marker
On Fri, 1 May 2020 10:19:42 -0400 Steven Rostedt rostedt@goodmis.org wrote:
On Fri, 1 May 2020 22:38:00 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yiding yidingx.liu@intel.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E" clear_trace() { # reset trace output echo > trace diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi echo "Test histogram trace_marker tigger" -echo 'hist:keys=common_pid' > events/ftrace/print/trigger +echo 'hist:keys=ip' > events/ftrace/print/trigger
This is doing more than just changing the echo being used. It's changing the test being done.
Yes, I need Tom's review for this change. As far as I can test, this fixes the test failure. If this isn't acceptable, we can use "alias echo=echo" for this test case.
Thank you,
for i in `seq 1 10` ; do echo "hello" > trace_marker; done grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \ fail "hist trigger did not trigger correct times on trace_marker" diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc index 18b4d1c2807e..c1625d945f4d 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc @@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
+# We have to use the built-in echo here because waking up pid must be same +# as echoing pid. +alias echo=echo sleep 1 echo "hello" > trace_marker diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc index dd262d6d0db6..23e52c8d71de 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc @@ -36,8 +36,8 @@ fi echo "Test histogram trace_marker to trace_marker latency histogram trigger" echo 'latency u64 lat' > synthetic_events -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
This too. And it's not explained in the change log why. In fact, these changes look like they belong in a separate patch.
-- Steve
echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger echo -n "start" > trace_marker echo -n "end" > trace_marker
On Sat, 2 May 2020 12:08:42 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi echo "Test histogram trace_marker tigger" -echo 'hist:keys=common_pid' > events/ftrace/print/trigger +echo 'hist:keys=ip' > events/ftrace/print/trigger
This is doing more than just changing the echo being used. It's changing the test being done.
Yes, I need Tom's review for this change. As far as I can test, this fixes the test failure. If this isn't acceptable, we can use "alias echo=echo" for this test case.
I still don't see how changing "keys=common_pid" to "keys=ip" has anything to do with the echo patch. If that is a problem, it should be a different patch with explanation to why "keys=common_pid" is broken.
-- Steve
On Thu, 7 May 2020 09:12:07 -0400 Steven Rostedt rostedt@goodmis.org wrote:
On Sat, 2 May 2020 12:08:42 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi echo "Test histogram trace_marker tigger" -echo 'hist:keys=common_pid' > events/ftrace/print/trigger +echo 'hist:keys=ip' > events/ftrace/print/trigger
This is doing more than just changing the echo being used. It's changing the test being done.
Yes, I need Tom's review for this change. As far as I can test, this fixes the test failure. If this isn't acceptable, we can use "alias echo=echo" for this test case.
I still don't see how changing "keys=common_pid" to "keys=ip" has anything to do with the echo patch. If that is a problem, it should be a different patch with explanation to why "keys=common_pid" is broken.
This test case uses a trace_marker event to make a histogram with the common_pid key, and it expects the "echo" command is built-in command so that the pid is same while writing several events to trace_marker. I changed it to "ip" which is always same if trace_marker interface is used.
Thank you,
-- Steve
On Fri, 8 May 2020 00:50:28 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Yes, I need Tom's review for this change. As far as I can test, this fixes the test failure. If this isn't acceptable, we can use "alias echo=echo" for this test case.
I still don't see how changing "keys=common_pid" to "keys=ip" has anything to do with the echo patch. If that is a problem, it should be a different patch with explanation to why "keys=common_pid" is broken.
This test case uses a trace_marker event to make a histogram with the common_pid key, and it expects the "echo" command is built-in command so that the pid is same while writing several events to trace_marker. I changed it to "ip" which is always same if trace_marker interface is used.
Can you explicitly state that in your change log? It wasn't obvious from what you meant with:
"This also fixes some test cases which expects built-in echo command."
Thanks!
-- Steve
Hi,
On 5/7/2020 12:25 PM, Steven Rostedt wrote:
On Fri, 8 May 2020 00:50:28 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Yes, I need Tom's review for this change. As far as I can test, this fixes the test failure. If this isn't acceptable, we can use "alias echo=echo" for this test case.
I still don't see how changing "keys=common_pid" to "keys=ip" has anything to do with the echo patch. If that is a problem, it should be a different patch with explanation to why "keys=common_pid" is broken.
This test case uses a trace_marker event to make a histogram with the common_pid key, and it expects the "echo" command is built-in command so that the pid is same while writing several events to trace_marker. I changed it to "ip" which is always same if trace_marker interface is used.
Can you explicitly state that in your change log? It wasn't obvious from what you meant with:
"This also fixes some test cases which expects built-in echo command."
With that change,
Reviewed-by: Tom Zanussi tom.zanussi@linux.intel.com
Thanks,
Tom
Thanks!
-- Steve
On Thu, 7 May 2020 15:32:46 -0500 "Zanussi, Tom" tom.zanussi@linux.intel.com wrote:
Hi,
On 5/7/2020 12:25 PM, Steven Rostedt wrote:
On Fri, 8 May 2020 00:50:28 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Yes, I need Tom's review for this change. As far as I can test, this fixes the test failure. If this isn't acceptable, we can use "alias echo=echo" for this test case.
I still don't see how changing "keys=common_pid" to "keys=ip" has anything to do with the echo patch. If that is a problem, it should be a different patch with explanation to why "keys=common_pid" is broken.
This test case uses a trace_marker event to make a histogram with the common_pid key, and it expects the "echo" command is built-in command so that the pid is same while writing several events to trace_marker. I changed it to "ip" which is always same if trace_marker interface is used.
Can you explicitly state that in your change log? It wasn't obvious from what you meant with:
"This also fixes some test cases which expects built-in echo command."
OK, will add the description.
With that change,
Reviewed-by: Tom Zanussi tom.zanussi@linux.intel.com
Thanks Tom!
Thanks,
Tom
Thanks!
-- Steve
On 2020/5/1 21:38, Masami Hiramatsu wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yidingyidingx.liu@intel.com Signed-off-by: Masami Hiramatsumhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E"
Hi Masami, Steven
It seems that only kprobe_syntax_errors.tc is impacted by the issue currently. Is it necessary for all tests to use /bin/echo and could we just make kprobe_syntax_errors.tc use /bin/echo?
Best Regards, Xiao Yang
clear_trace() { # reset trace output echo> trace diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi
echo "Test histogram trace_marker tigger"
-echo 'hist:keys=common_pid'> events/ftrace/print/trigger +echo 'hist:keys=ip'> events/ftrace/print/trigger for i in `seq 1 10` ; do echo "hello"> trace_marker; done grep 'hitcount: *10$' events/ftrace/print/hist> /dev/null || \ fail "hist trigger did not trigger correct times on trace_marker" diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc index 18b4d1c2807e..c1625d945f4d 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc @@ -44,6 +44,10 @@ echo 'latency u64 lat'> synthetic_events echo 'hist:keys=pid:ts0=common_timestamp.usecs'> events/sched/sched_waking/trigger echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
+# We have to use the built-in echo here because waking up pid must be same +# as echoing pid. +alias echo=echo sleep 1 echo "hello"> trace_marker
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc index dd262d6d0db6..23e52c8d71de 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc @@ -36,8 +36,8 @@ fi echo "Test histogram trace_marker to trace_marker latency histogram trigger"
echo 'latency u64 lat'> synthetic_events -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger echo -n "start"> trace_marker echo -n "end"> trace_marker
.
On Thu, 7 May 2020 14:45:16 +0800 Xiao Yang yangx.jy@cn.fujitsu.com wrote:
On 2020/5/1 21:38, Masami Hiramatsu wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yidingyidingx.liu@intel.com Signed-off-by: Masami Hiramatsumhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E"
Hi Masami, Steven
It seems that only kprobe_syntax_errors.tc is impacted by the issue currently. Is it necessary for all tests to use /bin/echo and could we just make kprobe_syntax_errors.tc use /bin/echo?
Yes, I would like to unify the "echo"'s behavior among the testcases instead of patching each failure in the future. Or would you have any concern on it?
Thank you,
Best Regards, Xiao Yang
clear_trace() { # reset trace output echo> trace diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi
echo "Test histogram trace_marker tigger"
-echo 'hist:keys=common_pid'> events/ftrace/print/trigger +echo 'hist:keys=ip'> events/ftrace/print/trigger for i in `seq 1 10` ; do echo "hello"> trace_marker; done grep 'hitcount: *10$' events/ftrace/print/hist> /dev/null || \ fail "hist trigger did not trigger correct times on trace_marker" diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc index 18b4d1c2807e..c1625d945f4d 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc @@ -44,6 +44,10 @@ echo 'latency u64 lat'> synthetic_events echo 'hist:keys=pid:ts0=common_timestamp.usecs'> events/sched/sched_waking/trigger echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
+# We have to use the built-in echo here because waking up pid must be same +# as echoing pid. +alias echo=echo sleep 1 echo "hello"> trace_marker
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc index dd262d6d0db6..23e52c8d71de 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc @@ -36,8 +36,8 @@ fi echo "Test histogram trace_marker to trace_marker latency histogram trigger"
echo 'latency u64 lat'> synthetic_events -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger echo -n "start"> trace_marker echo -n "end"> trace_marker
.
On 2020/5/7 17:15, Masami Hiramatsu wrote:
On Thu, 7 May 2020 14:45:16 +0800 Xiao Yangyangx.jy@cn.fujitsu.com wrote:
On 2020/5/1 21:38, Masami Hiramatsu wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yidingyidingx.liu@intel.com Signed-off-by: Masami Hiramatsumhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E"
Hi Masami, Steven
It seems that only kprobe_syntax_errors.tc is impacted by the issue currently. Is it necessary for all tests to use /bin/echo and could we just make kprobe_syntax_errors.tc use /bin/echo?
Yes, I would like to unify the "echo"'s behavior among the testcases instead of patching each failure in the future. Or would you have any concern on it?
Hi Masami,
Very sorry for the late reply.
We may not avoid fixing related failures after your change: 1) We have to reuse built-in echo (do alias echo=echo) if we want to test common_pid for histogram. 2) We have to reuse built-in echo if some new tests want to interpret backslash escapes in future.
Is it simple to provide two implementations of echo?(built-in echo and echo command?) and then just apply echo command for kprobe_syntax_errors.tc?
BTW: My suggestion may not be correct.
Best Regards, Xiao Yang
Thank you,
Best Regards, Xiao Yang
clear_trace() { # reset trace output echo> trace diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc index ab6bedb25736..b3f70f53ee69 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc @@ -30,7 +30,7 @@ fi
echo "Test histogram trace_marker tigger"
-echo 'hist:keys=common_pid'> events/ftrace/print/trigger +echo 'hist:keys=ip'> events/ftrace/print/trigger for i in `seq 1 10` ; do echo "hello"> trace_marker; done grep 'hitcount: *10$' events/ftrace/print/hist> /dev/null || \ fail "hist trigger did not trigger correct times on trace_marker" diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc index 18b4d1c2807e..c1625d945f4d 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc @@ -44,6 +44,10 @@ echo 'latency u64 lat'> synthetic_events echo 'hist:keys=pid:ts0=common_timestamp.usecs'> events/sched/sched_waking/trigger echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
+# We have to use the built-in echo here because waking up pid must be same +# as echoing pid. +alias echo=echo sleep 1 echo "hello"> trace_marker
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc index dd262d6d0db6..23e52c8d71de 100644 --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc @@ -36,8 +36,8 @@ fi echo "Test histogram trace_marker to trace_marker latency histogram trigger"
echo 'latency u64 lat'> synthetic_events -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger echo -n "start"> trace_marker echo -n "end"> trace_marker
.
On Mon, 11 May 2020 15:22:25 +0800 Xiao Yang yangx.jy@cn.fujitsu.com wrote:
On 2020/5/7 17:15, Masami Hiramatsu wrote:
On Thu, 7 May 2020 14:45:16 +0800 Xiao Yangyangx.jy@cn.fujitsu.com wrote:
On 2020/5/1 21:38, Masami Hiramatsu wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
This also fixes some test cases which expects built-in echo command.
Reported-by: Liu Yidingyidingx.liu@intel.com Signed-off-by: Masami Hiramatsumhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 5d4550591ff9..ea59b6ea2c3e 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -1,3 +1,6 @@ +# Since the built-in echo has different behavior in POSIX shell (dash) and +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes). +alias echo="/bin/echo -E"
Hi Masami, Steven
It seems that only kprobe_syntax_errors.tc is impacted by the issue currently. Is it necessary for all tests to use /bin/echo and could we just make kprobe_syntax_errors.tc use /bin/echo?
Yes, I would like to unify the "echo"'s behavior among the testcases instead of patching each failure in the future. Or would you have any concern on it?
Hi Masami,
Very sorry for the late reply.
We may not avoid fixing related failures after your change:
- We have to reuse built-in echo (do alias echo=echo) if we want to
test common_pid for histogram. 2) We have to reuse built-in echo if some new tests want to interpret backslash escapes in future.
1) yes, that's what I need to do for avoiding "pid" key histogram (but I think we should have better way to test it) 2) No, in that case you should use "/bin/echo -e" explicitly. dash's built-in echo doesn't support it.
Is it simple to provide two implementations of echo?(built-in echo and echo command?) and then just apply echo command for kprobe_syntax_errors.tc?
Hmm, OK, there might be another reason we reconsider this patch.
- Alisasing echo (this patch) can avoid dash related issues but this also makes "echo" running in another process implicitly.
- Using /bin/echo for backslash explicitly will be missed unless user runs it on dash, but it will keep "echo" in same process.
So both have pros/cons, but your idea will be locally effected. OK, I'll retry it.
Thank you,
From: Masami Hiramatsu
Sent: 11 May 2020 10:28
...
We may not avoid fixing related failures after your change:
- We have to reuse built-in echo (do alias echo=echo) if we want to
test common_pid for histogram. 2) We have to reuse built-in echo if some new tests want to interpret backslash escapes in future.
- yes, that's what I need to do for avoiding "pid" key histogram
(but I think we should have better way to test it) 2) No, in that case you should use "/bin/echo -e" explicitly. dash's built-in echo doesn't support it.
Is it simple to provide two implementations of echo?(built-in echo and echo command?) and then just apply echo command for kprobe_syntax_errors.tc?
Hmm, OK, there might be another reason we reconsider this patch.
Alisasing echo (this patch) can avoid dash related issues but this also makes "echo" running in another process implicitly.
Using /bin/echo for backslash explicitly will be missed unless user runs it on dash, but it will keep "echo" in same process.
Why not change to use printf - probably a builtin in both shells?
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Since the built-in echo has different behavior in POSIX shell (dash) and bash, kprobe_syntax_errors.tc can fail on dash which interpret backslash escape automatically.
To fix this issue, we explicitly use /bin/echo -E (not interpret backslash escapes) if the command string can include backslash.
Reported-by: Liu Yiding yidingx.liu@intel.com Suggested-by: Xiao Yang yangx.jy@cn.fujitsu.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- tools/testing/selftests/ftrace/test.d/functions | 8 +++++--- .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 61a3c7e2634d..69708830026f 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -119,12 +119,14 @@ yield() { ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1 }
+# Since probe event command may include backslash, explicitly use /bin/echo -E +# to NOT interpret it. ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file - pos=$(echo -n "${2%^*}" | wc -c) # error position - command=$(echo "$2" | tr -d ^) + pos=$(/bin/echo -En "${2%^*}" | wc -c) # error position + command=$(/bin/echo -E "$2" | tr -d ^) echo "Test command: $command" echo > error_log - (! echo "$command" >> "$3" ) 2> /dev/null + (! /bin/echo -E "$command" >> "$3" ) 2> /dev/null grep "$1: error:" -A 3 error_log N=$(tail -n 1 error_log | wc -c) # " Command: " and "^\n" => 13 diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc index ef1e9bafb098..4cfcf9440a1a 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc @@ -91,7 +91,9 @@ esac if grep -q "Create/append/" README && grep -q "imm-value" README; then echo 'p:kprobes/testevent _do_fork' > kprobe_events check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE -echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events + +# Explicitly use /bin/echo -E to not interpret \1 +/bin/echo -E 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd="foo"' # DIFF_ARG_TYPE
On Mai 11 2020, Masami Hiramatsu wrote:
To fix this issue, we explicitly use /bin/echo -E (not interpret backslash escapes) if the command string can include backslash.
Please use printf instead.
Andreas.
Since the built-in echo has different behavior in POSIX shell (dash) and bash, kprobe_syntax_errors.tc can fail on dash which interpret backslash escape automatically.
To fix this issue, we explicitly use printf "%s" (not interpret backslash escapes) if the command string can include backslash.
Reported-by: Liu Yiding yidingx.liu@intel.com Suggested-by: Xiao Yang yangx.jy@cn.fujitsu.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- tools/testing/selftests/ftrace/test.d/functions | 8 +++++--- .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 61a3c7e2634d..697c77ef2e2b 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -119,12 +119,14 @@ yield() { ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1 }
+# Since probe event command may include backslash, explicitly use printf "%s" +# to NOT interpret it. ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file - pos=$(echo -n "${2%^*}" | wc -c) # error position - command=$(echo "$2" | tr -d ^) + pos=$(printf "%s" "${2%^*}" | wc -c) # error position + command=$(printf "%s" "$2" | tr -d ^) echo "Test command: $command" echo > error_log - (! echo "$command" >> "$3" ) 2> /dev/null + (! printf "%s" "$command" >> "$3" ) 2> /dev/null grep "$1: error:" -A 3 error_log N=$(tail -n 1 error_log | wc -c) # " Command: " and "^\n" => 13 diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc index ef1e9bafb098..eb0f4ab4e070 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc @@ -91,7 +91,9 @@ esac if grep -q "Create/append/" README && grep -q "imm-value" README; then echo 'p:kprobes/testevent _do_fork' > kprobe_events check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE -echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events + +# Explicitly use printf "%s" to not interpret \1 +printf "%s" 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd="foo"' # DIFF_ARG_TYPE
Hi Andreas and David,
OK, what about this fix?
On Mon, 11 May 2020 22:36:27 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, kprobe_syntax_errors.tc can fail on dash which interpret backslash escape automatically.
To fix this issue, we explicitly use printf "%s" (not interpret backslash escapes) if the command string can include backslash.
Reported-by: Liu Yiding yidingx.liu@intel.com Suggested-by: Xiao Yang yangx.jy@cn.fujitsu.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 8 +++++--- .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 61a3c7e2634d..697c77ef2e2b 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -119,12 +119,14 @@ yield() { ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1 } +# Since probe event command may include backslash, explicitly use printf "%s" +# to NOT interpret it. ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
- pos=$(echo -n "${2%^*}" | wc -c) # error position
- command=$(echo "$2" | tr -d ^)
- pos=$(printf "%s" "${2%^*}" | wc -c) # error position
- command=$(printf "%s" "$2" | tr -d ^) echo "Test command: $command" echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null grep "$1: error:" -A 3 error_log N=$(tail -n 1 error_log | wc -c) # " Command: " and "^\n" => 13
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc index ef1e9bafb098..eb0f4ab4e070 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc @@ -91,7 +91,9 @@ esac if grep -q "Create/append/" README && grep -q "imm-value" README; then echo 'p:kprobes/testevent _do_fork' > kprobe_events check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE -echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
+# Explicitly use printf "%s" to not interpret \1 +printf "%s" 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd="foo"' # DIFF_ARG_TYPE
From: Masami Hiramatsu
Sent: 11 May 2020 14:38
Hi Andreas and David,
OK, what about this fix?
No idea what it is trying to do or why. Just a way of avoiding the differences between SYSV and BSD /bin/echo.
IIRC Posix allows both behaviours (and probably others).
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, 11 May 2020 13:46:35 +0000 David Laight David.Laight@ACULAB.COM wrote:
From: Masami Hiramatsu
Sent: 11 May 2020 14:38
Hi Andreas and David,
OK, what about this fix?
No idea what it is trying to do or why. Just a way of avoiding the differences between SYSV and BSD /bin/echo.
IIRC Posix allows both behaviours (and probably others).
Ah, I got it. That's why POSIX said "the results are implementation-defined."
https://pubs.opengroup.org/onlinepubs/009695399/utilities/echo.html
Thank you!
- pos=$(printf "%s" "${2%^*}" | wc -c) # error position
- command=$(printf "%s" "$2" | tr -d ^)
You may want to put all the $(...) inside "" to avoid field splitting (not relevant to a shell assignment with modern shells) and filename globbing.
echo "Test command: $command" echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null
WTF is the (! for ?? The (...) is a subshell. And ! inverts the exit status. Neither is needed at all.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, 11 May 2020 14:59:20 +0000 David Laight David.Laight@ACULAB.COM wrote:
echo "Test command: $command" echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null
WTF is the (! for ?? The (...) is a subshell. And ! inverts the exit status. Neither is needed at all.
This is done because the scripts are run with '-e' and will exit the script on any error.
This particular test is examining errors in the error log. The command being written into $3 is going to fail, and return an exit code. The "(! ..)" is needed, otherwise that failure will exit out the script.
-- Steve
On Mai 11 2020, Masami Hiramatsu wrote:
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null
printf %s does not print a newline, you need printf '%s\n' for that.
Andreas.
On Mon, 11 May 2020 15:42:10 +0200 Andreas Schwab schwab@linux-m68k.org wrote:
On Mai 11 2020, Masami Hiramatsu wrote:
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null
printf %s does not print a newline, you need printf '%s\n' for that.
Actually, ftrace doesn't need newline for single command. The reason why we had used echo instead of "echo -n" here, is just for short typing :)
Thank you,
Hi Shuah,
Could you pick this to kselftest-next?
Thank you,
On Mon, 11 May 2020 22:36:27 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, kprobe_syntax_errors.tc can fail on dash which interpret backslash escape automatically.
To fix this issue, we explicitly use printf "%s" (not interpret backslash escapes) if the command string can include backslash.
Reported-by: Liu Yiding yidingx.liu@intel.com Suggested-by: Xiao Yang yangx.jy@cn.fujitsu.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 8 +++++--- .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 61a3c7e2634d..697c77ef2e2b 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -119,12 +119,14 @@ yield() { ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1 } +# Since probe event command may include backslash, explicitly use printf "%s" +# to NOT interpret it. ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
- pos=$(echo -n "${2%^*}" | wc -c) # error position
- command=$(echo "$2" | tr -d ^)
- pos=$(printf "%s" "${2%^*}" | wc -c) # error position
- command=$(printf "%s" "$2" | tr -d ^) echo "Test command: $command" echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null grep "$1: error:" -A 3 error_log N=$(tail -n 1 error_log | wc -c) # " Command: " and "^\n" => 13
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc index ef1e9bafb098..eb0f4ab4e070 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc @@ -91,7 +91,9 @@ esac if grep -q "Create/append/" README && grep -q "imm-value" README; then echo 'p:kprobes/testevent _do_fork' > kprobe_events check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE -echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
+# Explicitly use printf "%s" to not interpret \1 +printf "%s" 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd="foo"' # DIFF_ARG_TYPE
On 5/25/20 3:59 AM, Masami Hiramatsu wrote:
Hi Shuah,
Could you pick this to kselftest-next?
Thank you,
On Mon, 11 May 2020 22:36:27 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, kprobe_syntax_errors.tc can fail on dash which interpret backslash escape automatically.
To fix this issue, we explicitly use printf "%s" (not interpret backslash escapes) if the command string can include backslash.
Reported-by: Liu Yiding yidingx.liu@intel.com Suggested-by: Xiao Yang yangx.jy@cn.fujitsu.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
tools/testing/selftests/ftrace/test.d/functions | 8 +++++--- .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions index 61a3c7e2634d..697c77ef2e2b 100644 --- a/tools/testing/selftests/ftrace/test.d/functions +++ b/tools/testing/selftests/ftrace/test.d/functions @@ -119,12 +119,14 @@ yield() { ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1 } +# Since probe event command may include backslash, explicitly use printf "%s" +# to NOT interpret it. ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
- pos=$(echo -n "${2%^*}" | wc -c) # error position
- command=$(echo "$2" | tr -d ^)
- pos=$(printf "%s" "${2%^*}" | wc -c) # error position
- command=$(printf "%s" "$2" | tr -d ^) echo "Test command: $command" echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
- (! printf "%s" "$command" >> "$3" ) 2> /dev/null grep "$1: error:" -A 3 error_log N=$(tail -n 1 error_log | wc -c) # " Command: " and "^\n" => 13
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc index ef1e9bafb098..eb0f4ab4e070 100644 --- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc @@ -91,7 +91,9 @@ esac if grep -q "Create/append/" README && grep -q "imm-value" README; then echo 'p:kprobes/testevent _do_fork' > kprobe_events check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE -echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
+# Explicitly use printf "%s" to not interpret \1 +printf "%s" 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE check_error 'p:kprobes/testevent _do_fork ^abcd="foo"' # DIFF_ARG_TYPE
Applied to
git.kernel.org/pub/scm/linux/kernel/git/shuah/linux kselftest.git/ next branch for Linux 5.8-rc1
thanks, -- Shuah
On Mai 01 2020, Masami Hiramatsu wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
How about using printf instead (at least where it matters)?
Andreas.
On Thu, 07 May 2020 11:22:28 +0200 Andreas Schwab schwab@linux-m68k.org wrote:
On Mai 01 2020, Masami Hiramatsu wrote:
Since the built-in echo has different behavior in POSIX shell (dash) and bash, we forcibly use /bin/echo -E (not interpret backslash escapes) by default.
How about using printf instead (at least where it matters)?
Hmm, I think replacing all echos with printf in the testcase might be much harder than this...
Thank you,
[ FYI, Shuah responds better from her linuxfoundation.org email ]
Shuah,
Feel free to take the first two patches of this series (I acked one, and reviewed the other).
The last patch I had some issues with, and is still under discussion.
Thanks!
-- Steve
On Fri, 1 May 2020 22:37:31 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
Hi,
Here are some patches to update ftracetest to fix some issues.
- [1/3] Fix coloring of XFAIL
- [2/3] Fix a testcase not to expect just one event entry
- [3/3] Do not use built-in echo because the behavior is different on dash and bash. (Thanks Liu for reporting!)
Thank you,
Masami Hiramatsu (3): selftests/ftrace: Make XFAIL green color selftests/ftrace: Pick only the first kprobe event to test selftests/ftrace: Use /bin/echo instead of built-in echo
tools/testing/selftests/ftrace/ftracetest | 2 +- tools/testing/selftests/ftrace/test.d/functions | 3 +++ .../ftrace/test.d/kprobe/kprobe_args_type.tc | 2 +- .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +- .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++ .../trigger/trigger-trace-marker-synthetic.tc | 4 ++-- 6 files changed, 12 insertions(+), 5 deletions(-)
-- Masami Hiramatsu (Linaro) mhiramat@kernel.org
On 5/1/20 8:21 AM, Steven Rostedt wrote:
[ FYI, Shuah responds better from her linuxfoundation.org email ]
Shuah,
Feel free to take the first two patches of this series (I acked one, and reviewed the other).
The last patch I had some issues with, and is still under discussion.
Thanks!
-- Steve
Will apply the first two.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Andreas Schwab -
David Laight -
Masami Hiramatsu -
shuah -
Shuah Khan -
Steven Rostedt -
Xiao Yang -
Zanussi, Tom