diff mbox series

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

Message ID 158920418730.16156.8299185499520876735.stgit@devnote2 (mailing list archive)
State Mainlined
Commit 8e923a2168afd221ea26e3d9716f21e9578b5c4d
Headers show
Series [v2] selftests/ftrace: Use printf for backslash included command | expand

Commit Message

Masami Hiramatsu (Google) May 11, 2020, 1:36 p.m. UTC
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(-)

Comments

Masami Hiramatsu (Google) May 11, 2020, 1:38 p.m. UTC | #1
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
>
Andreas Schwab May 11, 2020, 1:42 p.m. UTC | #2
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.
David Laight May 11, 2020, 1:46 p.m. UTC | #3
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)
Masami Hiramatsu (Google) May 11, 2020, 1:58 p.m. UTC | #4
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 (Google) May 11, 2020, 2:05 p.m. UTC | #5
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!
David Laight May 11, 2020, 2:59 p.m. UTC | #6
> > +    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)
Steven Rostedt May 11, 2020, 8:28 p.m. UTC | #7
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
Masami Hiramatsu (Google) May 25, 2020, 9:59 a.m. UTC | #8
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
>
Shuah Khan May 28, 2020, 4:24 p.m. UTC | #9
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
diff mbox series

Patch

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