[PATCH 0/3] selftests/ftrace: Fix ftracetest testcases for dash, etc.

[PATCH 0/3] selftests/ftrace: Fix ftracetest testcases for dash, etc.

[Masami Hiramatsu]

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>

[PATCH 1/3] selftests/ftrace: Make XFAIL green color

[Masami Hiramatsu]

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
     ;;

[PATCH 2/3] selftests/ftrace: Pick only the first kprobe event to test

[Masami Hiramatsu]

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"

[PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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

Re: [PATCH 1/3] selftests/ftrace: Make XFAIL green color

[Steven Rostedt]

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
>      ;;

Re: [PATCH 2/3] selftests/ftrace: Pick only the first kprobe event to test

[Steven Rostedt]

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

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Steven Rostedt]

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

Re: [PATCH 0/3] selftests/ftrace: Fix ftracetest testcases for dash, etc.

[Steven Rostedt]

[ 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>

Re: [PATCH 0/3] selftests/ftrace: Fix ftracetest testcases for dash, etc.

[Shuah Khan]

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

Re: [PATCH 2/3] selftests/ftrace: Pick only the first kprobe event to test

[shuah]

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/commit/?h=next&id=f0c0d0cf590f71b2213b29a7ded2cde3d0a1a0ba

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

Re: [PATCH 2/3] selftests/ftrace: Pick only the first kprobe event to test

[Steven Rostedt]

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.tc
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=f0c0d0cf590f71b2213b29a7ded2cde3d0a1a0ba
> 
> 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

Re: [PATCH 2/3] selftests/ftrace: Pick only the first kprobe event to test

[Shuah Khan]

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.tc
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=f0c0d0cf590f71b2213b29a7ded2cde3d0a1a0ba
>>
>> 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

Re: [PATCH 2/3] selftests/ftrace: Pick only the first kprobe event to test

[Masami Hiramatsu]

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.tc
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=next&id=f0c0d0cf590f71b2213b29a7ded2cde3d0a1a0ba
> 
> 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


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Xiao Yang]

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 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"
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
>
>
>
> .
>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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 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"
> 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
> >
> >
> >
> > .
> >
> 
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Steven Rostedt]

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

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Steven Rostedt]

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

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Zanussi, Tom]

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
> 

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Xiao Yang]

On 2020/5/7 17:15, Masami Hiramatsu wrote:
> 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 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"
>> 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
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>

Re: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[Masami Hiramatsu]

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 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 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"
> >> 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.

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,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

RE: [PATCH 3/3] selftests/ftrace: Use /bin/echo instead of built-in echo

[David Laight]

From: Masami Hiramatsu
> Sent: 11 May 2020 10:28
...
> > 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.
> 
> 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.

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)

[PATCH] selftests/ftrace: Use /bin/echo for backslash included command

[Masami Hiramatsu]

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

Re: [PATCH] selftests/ftrace: Use /bin/echo for backslash included command

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH v2] selftests/ftrace: Use printf for backslash included command

[Masami Hiramatsu]

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

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Masami Hiramatsu]

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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

RE: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[David Laight]

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)

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Masami Hiramatsu]

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,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Masami Hiramatsu]

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!

-- 
Masami Hiramatsu <mhiramat@kernel.org>

RE: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[David Laight]

> > +    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)

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Steven Rostedt]

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

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Masami Hiramatsu]

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
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] selftests/ftrace: Use printf for backslash included command

[Shuah Khan]

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