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 <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/testing/selftests/ftrace/test.d/functions | 3 +++
.../test.d/trigger/trigger-trace-marker-hist.tc | 2 +-
.../trigger-trace-marker-synthetic-kernel.tc | 4 ++++
.../trigger/trigger-trace-marker-synthetic.tc | 4 ++--
4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index 5d4550591ff9..ea59b6ea2c3e 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -1,3 +1,6 @@
+# Since the built-in echo has different behavior in POSIX shell (dash) and
+# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
+alias echo="/bin/echo -E"
clear_trace() { # reset trace output
echo > trace
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
index ab6bedb25736..b3f70f53ee69 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
@@ -30,7 +30,7 @@ fi
echo "Test histogram trace_marker tigger"
-echo 'hist:keys=common_pid' > events/ftrace/print/trigger
+echo 'hist:keys=ip' > events/ftrace/print/trigger
for i in `seq 1 10` ; do echo "hello" > trace_marker; done
grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \
fail "hist trigger did not trigger correct times on trace_marker"
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
index 18b4d1c2807e..c1625d945f4d 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
@@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events
echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger
echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger
echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
+
+# We have to use the built-in echo here because waking up pid must be same
+# as echoing pid.
+alias echo=echo
sleep 1
echo "hello" > trace_marker
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
index dd262d6d0db6..23e52c8d71de 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
@@ -36,8 +36,8 @@ fi
echo "Test histogram trace_marker to trace_marker latency histogram trigger"
echo 'latency u64 lat' > synthetic_events
-echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
-echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
+echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
+echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
echo -n "start" > trace_marker
echo -n "end" > trace_marker
On Fri, 1 May 2020 22:38:00 +0900
Masami Hiramatsu <[email protected]> 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 <[email protected]>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> tools/testing/selftests/ftrace/test.d/functions | 3 +++
> .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +-
> .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++
> .../trigger/trigger-trace-marker-synthetic.tc | 4 ++--
> 4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 5d4550591ff9..ea59b6ea2c3e 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -1,3 +1,6 @@
> +# Since the built-in echo has different behavior in POSIX shell (dash) and
> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> +alias echo="/bin/echo -E"
>
> clear_trace() { # reset trace output
> echo > trace
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> index ab6bedb25736..b3f70f53ee69 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> @@ -30,7 +30,7 @@ fi
>
> echo "Test histogram trace_marker tigger"
>
> -echo 'hist:keys=common_pid' > events/ftrace/print/trigger
> +echo 'hist:keys=ip' > events/ftrace/print/trigger
This is doing more than just changing the echo being used. It's changing
the test being done.
> for i in `seq 1 10` ; do echo "hello" > trace_marker; done
> grep 'hitcount: *10$' events/ftrace/print/hist > /dev/null || \
> fail "hist trigger did not trigger correct times on trace_marker"
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> index 18b4d1c2807e..c1625d945f4d 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> @@ -44,6 +44,10 @@ echo 'latency u64 lat' > synthetic_events
> echo 'hist:keys=pid:ts0=common_timestamp.usecs' > events/sched/sched_waking/trigger
> echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)' > events/ftrace/print/trigger
> echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
> +
> +# We have to use the built-in echo here because waking up pid must be same
> +# as echoing pid.
> +alias echo=echo
> sleep 1
> echo "hello" > trace_marker
>
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> index dd262d6d0db6..23e52c8d71de 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> @@ -36,8 +36,8 @@ fi
> echo "Test histogram trace_marker to trace_marker latency histogram trigger"
>
> echo 'latency u64 lat' > synthetic_events
> -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
> -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
> +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"' > events/ftrace/print/trigger
> +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"' >> events/ftrace/print/trigger
This too. And it's not explained in the change log why. In fact, these
changes look like they belong in a separate patch.
-- Steve
> echo 'hist:keys=common_pid,lat:sort=lat' > events/synthetic/latency/trigger
> echo -n "start" > trace_marker
> echo -n "end" > trace_marker
On Fri, 1 May 2020 10:19:42 -0400
Steven Rostedt <[email protected]> wrote:
> On Fri, 1 May 2020 22:38:00 +0900
> Masami Hiramatsu <[email protected]> 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 <[email protected]>
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > ---
> > 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 <[email protected]>
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<[email protected]>
> Signed-off-by: Masami Hiramatsu<[email protected]>
> ---
> tools/testing/selftests/ftrace/test.d/functions | 3 +++
> .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +-
> .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++
> .../trigger/trigger-trace-marker-synthetic.tc | 4 ++--
> 4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 5d4550591ff9..ea59b6ea2c3e 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -1,3 +1,6 @@
> +# Since the built-in echo has different behavior in POSIX shell (dash) and
> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
> +alias echo="/bin/echo -E"
Hi Masami, Steven
It seems that only kprobe_syntax_errors.tc is impacted by the issue
currently. Is it necessary for all tests to use /bin/echo and could we
just make kprobe_syntax_errors.tc use /bin/echo?
Best Regards,
Xiao Yang
>
> clear_trace() { # reset trace output
> echo> trace
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> index ab6bedb25736..b3f70f53ee69 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> @@ -30,7 +30,7 @@ fi
>
> echo "Test histogram trace_marker tigger"
>
> -echo 'hist:keys=common_pid'> events/ftrace/print/trigger
> +echo 'hist:keys=ip'> events/ftrace/print/trigger
> for i in `seq 1 10` ; do echo "hello"> trace_marker; done
> grep 'hitcount: *10$' events/ftrace/print/hist> /dev/null || \
> fail "hist trigger did not trigger correct times on trace_marker"
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> index 18b4d1c2807e..c1625d945f4d 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
> @@ -44,6 +44,10 @@ echo 'latency u64 lat'> synthetic_events
> echo 'hist:keys=pid:ts0=common_timestamp.usecs'> events/sched/sched_waking/trigger
> echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'> events/ftrace/print/trigger
> echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
> +
> +# We have to use the built-in echo here because waking up pid must be same
> +# as echoing pid.
> +alias echo=echo
> sleep 1
> echo "hello"> trace_marker
>
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> index dd262d6d0db6..23e52c8d71de 100644
> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
> @@ -36,8 +36,8 @@ fi
> echo "Test histogram trace_marker to trace_marker latency histogram trigger"
>
> echo 'latency u64 lat'> synthetic_events
> -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger
> -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger
> +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger
> +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger
> echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
> echo -n "start"> trace_marker
> echo -n "end"> trace_marker
>
>
>
> .
>
On Thu, 7 May 2020 14:45:16 +0800
Xiao Yang <[email protected]> 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<[email protected]>
> > Signed-off-by: Masami Hiramatsu<[email protected]>
> > ---
> > 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 <[email protected]>
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, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Sat, 2 May 2020 12:08:42 +0900
Masami Hiramatsu <[email protected]> wrote:
> > > diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > index ab6bedb25736..b3f70f53ee69 100644
> > > --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
> > > @@ -30,7 +30,7 @@ fi
> > >
> > > echo "Test histogram trace_marker tigger"
> > >
> > > -echo 'hist:keys=common_pid' > events/ftrace/print/trigger
> > > +echo 'hist:keys=ip' > events/ftrace/print/trigger
> >
> > This is doing more than just changing the echo being used. It's changing
> > the test being done.
>
> Yes, I need Tom's review for this change. As far as I can test, this
> fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
> for this test case.
>
I still don't see how changing "keys=common_pid" to "keys=ip" has anything
to do with the echo patch. If that is a problem, it should be a different
patch with explanation to why "keys=common_pid" is broken.
-- Steve
On Thu, 7 May 2020 09:12:07 -0400
Steven Rostedt <[email protected]> wrote:
> On Sat, 2 May 2020 12:08:42 +0900
> Masami Hiramatsu <[email protected]> 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 <[email protected]>
On Thu, 07 May 2020 11:22:28 +0200
Andreas Schwab <[email protected]> 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 <[email protected]>
On Fri, 8 May 2020 00:50:28 +0900
Masami Hiramatsu <[email protected]> wrote:
> > > Yes, I need Tom's review for this change. As far as I can test, this
> > > fixes the test failure. If this isn't acceptable, we can use "alias echo=echo"
> > > for this test case.
> > >
> >
> > I still don't see how changing "keys=common_pid" to "keys=ip" has anything
> > to do with the echo patch. If that is a problem, it should be a different
> > patch with explanation to why "keys=common_pid" is broken.
>
> This test case uses a trace_marker event to make a histogram with
> the common_pid key, and it expects the "echo" command is built-in command
> so that the pid is same while writing several events to trace_marker.
> I changed it to "ip" which is always same if trace_marker interface is
> used.
Can you explicitly state that in your change log? It wasn't obvious from
what you meant with:
"This also fixes some test cases which expects built-in echo command."
Thanks!
-- Steve
Hi,
On 5/7/2020 12:25 PM, Steven Rostedt wrote:
> On Fri, 8 May 2020 00:50:28 +0900
> Masami Hiramatsu <[email protected]> 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 <[email protected]>
Thanks,
Tom
> Thanks!
>
> -- Steve
>
On Thu, 7 May 2020 15:32:46 -0500
"Zanussi, Tom" <[email protected]> wrote:
> Hi,
>
> On 5/7/2020 12:25 PM, Steven Rostedt wrote:
> > On Fri, 8 May 2020 00:50:28 +0900
> > Masami Hiramatsu <[email protected]> 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 <[email protected]>
Thanks Tom!
>
> Thanks,
>
> Tom
>
> > Thanks!
> >
> > -- Steve
> >
--
Masami Hiramatsu <[email protected]>
On 2020/5/7 17:15, Masami Hiramatsu wrote:
> On Thu, 7 May 2020 14:45:16 +0800
> Xiao Yang<[email protected]> 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<[email protected]>
>>> Signed-off-by: Masami Hiramatsu<[email protected]>
>>> ---
>>> tools/testing/selftests/ftrace/test.d/functions | 3 +++
>>> .../test.d/trigger/trigger-trace-marker-hist.tc | 2 +-
>>> .../trigger-trace-marker-synthetic-kernel.tc | 4 ++++
>>> .../trigger/trigger-trace-marker-synthetic.tc | 4 ++--
>>> 4 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
>>> index 5d4550591ff9..ea59b6ea2c3e 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/functions
>>> +++ b/tools/testing/selftests/ftrace/test.d/functions
>>> @@ -1,3 +1,6 @@
>>> +# Since the built-in echo has different behavior in POSIX shell (dash) and
>>> +# bash, we forcibly use /bin/echo -E (not interpret backslash escapes).
>>> +alias echo="/bin/echo -E"
>> Hi Masami, Steven
>>
>> It seems that only kprobe_syntax_errors.tc is impacted by the issue
>> currently. Is it necessary for all tests to use /bin/echo and could we
>> just make kprobe_syntax_errors.tc use /bin/echo?
>
> Yes, I would like to unify the "echo"'s behavior among the testcases
> instead of patching each failure in the future.
> Or would you have any concern on it?
Hi Masami,
Very sorry for the late reply.
We may not avoid fixing related failures after your change:
1) We have to reuse built-in echo (do alias echo=echo) if we want to
test common_pid for histogram.
2) We have to reuse built-in echo if some new tests want to interpret
backslash escapes in future.
Is it simple to provide two implementations of echo?(built-in echo and
echo command?) and then just apply echo command for kprobe_syntax_errors.tc?
BTW: My suggestion may not be correct.
Best Regards,
Xiao Yang
>
> Thank you,
>
>>
>> Best Regards,
>> Xiao Yang
>>
>>>
>>> clear_trace() { # reset trace output
>>> echo> trace
>>> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
>>> index ab6bedb25736..b3f70f53ee69 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
>>> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-hist.tc
>>> @@ -30,7 +30,7 @@ fi
>>>
>>> echo "Test histogram trace_marker tigger"
>>>
>>> -echo 'hist:keys=common_pid'> events/ftrace/print/trigger
>>> +echo 'hist:keys=ip'> events/ftrace/print/trigger
>>> for i in `seq 1 10` ; do echo "hello"> trace_marker; done
>>> grep 'hitcount: *10$' events/ftrace/print/hist> /dev/null || \
>>> fail "hist trigger did not trigger correct times on trace_marker"
>>> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
>>> index 18b4d1c2807e..c1625d945f4d 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
>>> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic-kernel.tc
>>> @@ -44,6 +44,10 @@ echo 'latency u64 lat'> synthetic_events
>>> echo 'hist:keys=pid:ts0=common_timestamp.usecs'> events/sched/sched_waking/trigger
>>> echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(sched.sched_waking).latency($lat)'> events/ftrace/print/trigger
>>> echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
>>> +
>>> +# We have to use the built-in echo here because waking up pid must be same
>>> +# as echoing pid.
>>> +alias echo=echo
>>> sleep 1
>>> echo "hello"> trace_marker
>>>
>>> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
>>> index dd262d6d0db6..23e52c8d71de 100644
>>> --- a/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
>>> +++ b/tools/testing/selftests/ftrace/test.d/trigger/trigger-trace-marker-synthetic.tc
>>> @@ -36,8 +36,8 @@ fi
>>> echo "Test histogram trace_marker to trace_marker latency histogram trigger"
>>>
>>> echo 'latency u64 lat'> synthetic_events
>>> -echo 'hist:keys=common_pid:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger
>>> -echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger
>>> +echo 'hist:keys=ip:ts0=common_timestamp.usecs if buf == "start"'> events/ftrace/print/trigger
>>> +echo 'hist:keys=ip:lat=common_timestamp.usecs-$ts0:onmatch(ftrace.print).latency($lat) if buf == "end"'>> events/ftrace/print/trigger
>>> echo 'hist:keys=common_pid,lat:sort=lat'> events/synthetic/latency/trigger
>>> echo -n "start"> trace_marker
>>> echo -n "end"> trace_marker
>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>
On Mon, 11 May 2020 15:22:25 +0800
Xiao Yang <[email protected]> wrote:
> On 2020/5/7 17:15, Masami Hiramatsu wrote:
> > On Thu, 7 May 2020 14:45:16 +0800
> > Xiao Yang<[email protected]> 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<[email protected]>
> >>> Signed-off-by: Masami Hiramatsu<[email protected]>
> >>> ---
> >>> 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 <[email protected]>
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)
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 <[email protected]>
Suggested-by: Xiao Yang <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/testing/selftests/ftrace/test.d/functions | 8 +++++---
.../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index 61a3c7e2634d..69708830026f 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -119,12 +119,14 @@ yield() {
ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1
}
+# Since probe event command may include backslash, explicitly use /bin/echo -E
+# to NOT interpret it.
ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
- pos=$(echo -n "${2%^*}" | wc -c) # error position
- command=$(echo "$2" | tr -d ^)
+ pos=$(/bin/echo -En "${2%^*}" | wc -c) # error position
+ command=$(/bin/echo -E "$2" | tr -d ^)
echo "Test command: $command"
echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
+ (! /bin/echo -E "$command" >> "$3" ) 2> /dev/null
grep "$1: error:" -A 3 error_log
N=$(tail -n 1 error_log | wc -c)
# " Command: " and "^\n" => 13
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
index ef1e9bafb098..4cfcf9440a1a 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
@@ -91,7 +91,9 @@ esac
if grep -q "Create/append/" README && grep -q "imm-value" README; then
echo 'p:kprobes/testevent _do_fork' > kprobe_events
check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE
-echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
+
+# Explicitly use /bin/echo -E to not interpret \1
+/bin/echo -E 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE
check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE
check_error 'p:kprobes/testevent _do_fork ^abcd=\"foo"' # DIFF_ARG_TYPE
On Mai 11 2020, Masami Hiramatsu wrote:
> To fix this issue, we explicitly use /bin/echo -E (not interpret
> backslash escapes) if the command string can include backslash.
Please use printf instead.
Andreas.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
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 <[email protected]>
Suggested-by: Xiao Yang <[email protected]>
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/testing/selftests/ftrace/test.d/functions | 8 +++++---
.../ftrace/test.d/kprobe/kprobe_syntax_errors.tc | 4 +++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index 61a3c7e2634d..697c77ef2e2b 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -119,12 +119,14 @@ yield() {
ping $LOCALHOST -c 1 || sleep .001 || usleep 1 || sleep 1
}
+# Since probe event command may include backslash, explicitly use printf "%s"
+# to NOT interpret it.
ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
- pos=$(echo -n "${2%^*}" | wc -c) # error position
- command=$(echo "$2" | tr -d ^)
+ pos=$(printf "%s" "${2%^*}" | wc -c) # error position
+ command=$(printf "%s" "$2" | tr -d ^)
echo "Test command: $command"
echo > error_log
- (! echo "$command" >> "$3" ) 2> /dev/null
+ (! printf "%s" "$command" >> "$3" ) 2> /dev/null
grep "$1: error:" -A 3 error_log
N=$(tail -n 1 error_log | wc -c)
# " Command: " and "^\n" => 13
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
index ef1e9bafb098..eb0f4ab4e070 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
@@ -91,7 +91,9 @@ esac
if grep -q "Create/append/" README && grep -q "imm-value" README; then
echo 'p:kprobes/testevent _do_fork' > kprobe_events
check_error '^r:kprobes/testevent do_exit' # DIFF_PROBE_TYPE
-echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
+
+# Explicitly use printf "%s" to not interpret \1
+printf "%s" 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
check_error 'p:kprobes/testevent _do_fork ^bcd=\1' # DIFF_ARG_TYPE
check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8' # DIFF_ARG_TYPE
check_error 'p:kprobes/testevent _do_fork ^abcd=\"foo"' # DIFF_ARG_TYPE
Hi Andreas and David,
OK, what about this fix?
On Mon, 11 May 2020 22:36:27 +0900
Masami Hiramatsu <[email protected]> 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 <[email protected]>
> Suggested-by: Xiao Yang <[email protected]>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> 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 <[email protected]>
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, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
From: Masami Hiramatsu
> Sent: 11 May 2020 14:38
>
> Hi Andreas and David,
>
> OK, what about this fix?
No idea what it is trying to do or why.
Just a way of avoiding the differences between SYSV and BSD /bin/echo.
IIRC Posix allows both behaviours (and probably others).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, 11 May 2020 15:42:10 +0200
Andreas Schwab <[email protected]> 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 <[email protected]>
On Mon, 11 May 2020 13:46:35 +0000
David Laight <[email protected]> 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 <[email protected]>
> > + pos=$(printf "%s" "${2%^*}" | wc -c) # error position
> > + command=$(printf "%s" "$2" | tr -d ^)
You may want to put all the $(...) inside "" to avoid field splitting
(not relevant to a shell assignment with modern shells) and
filename globbing.
> > echo "Test command: $command"
> > echo > error_log
> > - (! echo "$command" >> "$3" ) 2> /dev/null
> > + (! printf "%s" "$command" >> "$3" ) 2> /dev/null
WTF is the (! for ??
The (...) is a subshell.
And ! inverts the exit status.
Neither is needed at all.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, 11 May 2020 14:59:20 +0000
David Laight <[email protected]> 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
Hi Shuah,
Could you pick this to kselftest-next?
Thank you,
On Mon, 11 May 2020 22:36:27 +0900
Masami Hiramatsu <[email protected]> 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 <[email protected]>
> Suggested-by: Xiao Yang <[email protected]>
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>> Suggested-by: Xiao Yang <[email protected]>
>> Signed-off-by: Masami Hiramatsu <[email protected]>
>> ---
>> 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