Re: [PATCH v3 0/7] tracing: Synthetic event dynamic string fixes

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Tom,

On Tue, 13 Oct 2020 09:17:51 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> This updates v2 to replace some of the v2 code with improved code from
> Steve (tracing: Add synthetic event error logging) and (tracing:
> Handle synthetic event array field type checking correctly) and remove
> the synth_error_clear() function and change last_cmd_set() to use
> strncpy.

Thank you for updating, I tested the series and confirmed all issues 
are fixed now :)

Tested-by: Masami Hiramatsu <mhiramat@kernel.org>

for this series.

/ # cd /sys/kernel/debug/tracing/
/sys/kernel/debug/tracing # echo 'myevent char foo]' >> synthetic_events 
sh: write error: Invalid argument
/sys/kernel/debug/tracing # cat error_log 
[   35.086780] synthetic_events: error: Illegal name
  Command: myevent char foo]
                        ^
/sys/kernel/debug/tracing # echo 'myevent char foo;[]' >> synthetic_events 
sh: write error: Invalid argument
/sys/kernel/debug/tracing # cat error_log 
[   35.086780] synthetic_events: error: Illegal name
  Command: myevent char foo]
                        ^
[   46.857674] synthetic_events: error: Illegal name
  Command: myevent char foo;[]
                        ^
/sys/kernel/debug/tracing # echo 'myevent char foo+[]' >> synthetic_events 
sh: write error: Invalid argument
/sys/kernel/debug/tracing # cat error_log 
[   35.086780] synthetic_events: error: Illegal name
  Command: myevent char foo]
                        ^
[   46.857674] synthetic_events: error: Illegal name
  Command: myevent char foo;[]
                        ^
[   57.220147] synthetic_events: error: Illegal name
  Command: myevent char foo+[]
                        ^
/sys/kernel/debug/tracing # echo 'myevent char foo[]' >> synthetic_events 
/sys/kernel/debug/tracing # cat synthetic_events 
myevent	char[] foo
/sys/kernel/debug/tracing # echo > synthetic_events 
/sys/kernel/debug/tracing # echo 'myevent char[] foo' >> synthetic_events 
/sys/kernel/debug/tracing # cat synthetic_events 
myevent	char[] foo
/sys/kernel/debug/tracing # cat events/synthetic/myevent/format 
name: myevent
ID: 1219
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:__data_loc char[] foo;	offset:8;	size:8;	signed:1;

print fmt: "foo=%.*s", __get_str(foo)


> 
> Thanks,
> 
> Tom
> 
> v2 text:
> 
> This updates v1 to fix a couple of additional things that Masami
> pointed out:
> 
>  - The error logging for the BAD_TYPE error was actually pointing to
>    the name - it now points to the type as it should.
> 
>  - Added a new test case that verifies most of the synthetic event
>    error messages and caret positions.
>  
>  - Added a new patch that correctly strips off trailing semicolons and
>    everything else from array types, which wasn't happening before,
>    even before the dynamic array patches.
> 
> Original v1 text:
> 
> These patches provide fixes for the problems observed by Masami in the
> new synthetic event dynamic string patchset.
> 
> The first patch (tracing: Don't show dynamic string internals in
> synthetic event description) removes the __data_loc from the event
> description but leaves it in the format.
> 
> The patch (tracing: Add synthetic event error logging) addresses the
> lack of error messages when parse errors occur.
> 
> The remaining three patches address the other problems Masami noted
> which result from allowing illegal characters in synthetic event and
> field names when defining an event.  The is_good_name() function is
> used to check that's not possible for the probe events, but should
> also be used for the synthetic events as well.
> 
> (tracing: Move is_good_name() from trace_probe.h to trace.h) makes
> that function available to other trace subsystems by putting it in
> trace.h.  (tracing: Check that the synthetic event and field names are
> legal) applies it to the synthetic events, and (selftests/ftrace:
> Change synthetic event name for inter-event-combined test) changes a
> testcase that now fails because it uses an illegal name.
> 
> The following changes since commit 848183553e431e6e9c2ea2f72421a7a1bbc6532e:
> 
>   tracing: Fix synthetic print fmt check for use of __get_str() (2020-10-08 15:29:07 -0400)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/dynstring-fixes-v3
> 
> Tom Zanussi (7):
>   tracing: Don't show dynamic string internals in synthetic event
>     description
>   tracing: Move is_good_name() from trace_probe.h to trace.h
>   tracing: Check that the synthetic event and field names are legal
>   tracing: Add synthetic event error logging
>   selftests/ftrace: Change synthetic event name for inter-event-combined
>     test
>   tracing: Handle synthetic event array field type checking correctly
>   selftests/ftrace: Add test case for synthetic event syntax errors
> 
>  kernel/trace/trace.h                          |  13 ++
>  kernel/trace/trace_events_synth.c             | 123 +++++++++++++++++-
>  kernel/trace/trace_probe.h                    |  13 --
>  .../trigger-inter-event-combined-hist.tc      |   8 +-
>  .../trigger-synthetic_event_syntax_errors.tc  |  19 +++
>  5 files changed, 153 insertions(+), 23 deletions(-)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
> 
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>