[PATCH v2 0/3] tracing: Fix synthetic event parser

[PATCH v2 0/3] tracing: Fix synthetic event parser

[Masami Hiramatsu]

Hi,

Here are 2 fixes for synthetic event parser and its testcase.
One bug is that the synthetic_events can not accept "unsigned"
modifier for the field type, and another one is not allowing
the last independent semicolon.

I found that there is no testcase for checking it too. So the
last patch in this series adding a testcase for synthetic
event parser.

Thank you,

---

Masami Hiramatsu (3):
      tracing: Fix synthetic event to accept unsigned modifier
      tracing: Fix synthetic event to allow semicolon at end
      selftests: ftrace: Add synthetic event syntax testcase


 kernel/trace/trace_events_hist.c                   |   32 ++++++--
 .../inter-event/trigger-synthetic-event-syntax.tc  |   80 ++++++++++++++++++++
 2 files changed, 105 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH v2 1/3] tracing: Fix synthetic event to accept unsigned modifier

[Masami Hiramatsu]

Fix synthetic event to accept unsigned modifier for its field type
correctly.

Currently, synthetic_events interface returns error for "unsigned"
modifiers as below;

 # echo "myevent unsigned long var" >> synthetic_events
 sh: write error: Invalid argument

This is because argv_split() breaks "unsigned long" into "unsigned"
and "long", but parse_synth_field() doesn't expected it.

With this fix, synthetic_events can handle the "unsigned long"
correctly like as below;

 # echo "myevent unsigned long var" >> synthetic_events
 # cat synthetic_events
 myevent	unsigned long var

Fixes: commit 4b147936fa50 ("tracing: Add support for 'synthetic' events")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
Cc: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 Changes in v2:
  - Fix a typo of stable ML.
---
 kernel/trace/trace_events_hist.c |   30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 85f6b01431c7..6ff83941065a 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -738,16 +738,30 @@ static void free_synth_field(struct synth_field *field)
 	kfree(field);
 }
 
-static struct synth_field *parse_synth_field(char *field_type,
-					     char *field_name)
+static struct synth_field *parse_synth_field(int argc, char **argv,
+					     int *consumed)
 {
 	struct synth_field *field;
+	const char *prefix = NULL;
+	char *field_type = argv[0], *field_name;
 	int len, ret = 0;
 	char *array;
 
 	if (field_type[0] == ';')
 		field_type++;
 
+	if (!strcmp(field_type, "unsigned")) {
+		if (argc < 3)
+			return ERR_PTR(-EINVAL);
+		prefix = "unsigned ";
+		field_type = argv[1];
+		field_name = argv[2];
+		*consumed = 3;
+	} else {
+		field_name = argv[1];
+		*consumed = 2;
+	}
+
 	len = strlen(field_name);
 	if (field_name[len - 1] == ';')
 		field_name[len - 1] = '\0';
@@ -760,11 +774,15 @@ static struct synth_field *parse_synth_field(char *field_type,
 	array = strchr(field_name, '[');
 	if (array)
 		len += strlen(array);
+	if (prefix)
+		len += strlen(prefix);
 	field->type = kzalloc(len, GFP_KERNEL);
 	if (!field->type) {
 		ret = -ENOMEM;
 		goto free;
 	}
+	if (prefix)
+		strcat(field->type, prefix);
 	strcat(field->type, field_type);
 	if (array) {
 		strcat(field->type, array);
@@ -1009,7 +1027,7 @@ static int create_synth_event(int argc, char **argv)
 	struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
 	struct synth_event *event = NULL;
 	bool delete_event = false;
-	int i, n_fields = 0, ret = 0;
+	int i, consumed = 0, n_fields = 0, ret = 0;
 	char *name;
 
 	mutex_lock(&synth_event_mutex);
@@ -1061,13 +1079,13 @@ static int create_synth_event(int argc, char **argv)
 			goto err;
 		}
 
-		field = parse_synth_field(argv[i], argv[i + 1]);
+		field = parse_synth_field(argc - i, &argv[i], &consumed);
 		if (IS_ERR(field)) {
 			ret = PTR_ERR(field);
 			goto err;
 		}
-		fields[n_fields] = field;
-		i++; n_fields++;
+		fields[n_fields++] = field;
+		i += consumed - 1;
 	}
 
 	if (i < argc) {

[PATCH v2 2/3] tracing: Fix synthetic event to allow semicolon at end

[Masami Hiramatsu]

Fix synthetic event to allow independent semicolon at end.

The synthetic_events interface accepts a semicolon after the
last word if there is no space.

 # echo "myevent u64 var;" >> synthetic_events

But if there is a space, it returns an error.

 # echo "myevent u64 var ;" > synthetic_events
 sh: write error: Invalid argument

This behavior is difficult for users to understand. Let's
allow the last independent semicolon too.

Fixes: commit 4b147936fa50 ("tracing: Add support for 'synthetic' events")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
Cc: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 6ff83941065a..d239004aaf29 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1088,7 +1088,7 @@ static int create_synth_event(int argc, char **argv)
 		i += consumed - 1;
 	}
 
-	if (i < argc) {
+	if (i < argc && strcmp(argv[i], ";") != 0) {
 		ret = -EINVAL;
 		goto err;
 	}

[PATCH v2 3/3] selftests: ftrace: Add synthetic event syntax testcase

[Masami Hiramatsu]

Add a testcase to check the syntax and field types for
synthetic_events interface.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 .../inter-event/trigger-synthetic-event-syntax.tc  |   80 ++++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc

diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc
new file mode 100644
index 000000000000..88e6c3f43006
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc
@@ -0,0 +1,80 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: event trigger - test synthetic_events syntax parser
+
+do_reset() {
+    reset_trigger
+    echo > set_event
+    clear_trace
+}
+
+fail() { #msg
+    do_reset
+    echo $1
+    exit_fail
+}
+
+if [ ! -f set_event ]; then
+    echo "event tracing is not supported"
+    exit_unsupported
+fi
+
+if [ ! -f synthetic_events ]; then
+    echo "synthetic event is not supported"
+    exit_unsupported
+fi
+
+reset_tracer
+do_reset
+
+echo "Test synthetic_events syntax parser"
+
+echo > synthetic_events
+
+# synthetic event must have a field
+! echo "myevent" >> synthetic_events
+echo "myevent u64 var1" >> synthetic_events
+
+# synthetic event must be found in synthetic_events
+grep "myevent[[:space:]]u64 var1" synthetic_events
+
+# it is not possible to add same name event
+! echo "myevent u64 var2" >> synthetic_events
+
+# Non-append open will cleanup all events and add new one
+echo "myevent u64 var2" > synthetic_events
+
+# multiple fields with different spaces
+echo "myevent u64 var1; u64 var2;" > synthetic_events
+grep "myevent[[:space:]]u64 var1; u64 var2" synthetic_events
+echo "myevent u64 var1 ; u64 var2 ;" > synthetic_events
+grep "myevent[[:space:]]u64 var1; u64 var2" synthetic_events
+echo "myevent u64 var1 ;u64 var2" > synthetic_events
+grep "myevent[[:space:]]u64 var1; u64 var2" synthetic_events
+
+# test field types
+echo "myevent u32 var" > synthetic_events
+echo "myevent u16 var" > synthetic_events
+echo "myevent u8 var" > synthetic_events
+echo "myevent s64 var" > synthetic_events
+echo "myevent s32 var" > synthetic_events
+echo "myevent s16 var" > synthetic_events
+echo "myevent s8 var" > synthetic_events
+
+echo "myevent char var" > synthetic_events
+echo "myevent int var" > synthetic_events
+echo "myevent long var" > synthetic_events
+echo "myevent pid_t var" > synthetic_events
+
+echo "myevent unsigned char var" > synthetic_events
+echo "myevent unsigned int var" > synthetic_events
+echo "myevent unsigned long var" > synthetic_events
+grep "myevent[[:space:]]unsigned long var" synthetic_events
+
+# test string type
+echo "myevent char var[10]" > synthetic_events
+grep "myevent[[:space:]]char\[10\] var" synthetic_events
+
+do_reset
+
+exit 0

Re: [PATCH v2 3/3] selftests: ftrace: Add synthetic event syntax testcase

[Steven Rostedt]

Hi Masami,

Thanks! I pulled this into my urgent queue and will be running tests on
it over night.

Shuah, can you ack this patch, as I want to add it to this release,
along with the other changes.

Thanks!

-- Steve


On Thu, 18 Oct 2018 22:13:02 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Add a testcase to check the syntax and field types for
> synthetic_events interface.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  .../inter-event/trigger-synthetic-event-syntax.tc  |   80 ++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc
> new file mode 100644
> index 000000000000..88e6c3f43006
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic-event-syntax.tc
> @@ -0,0 +1,80 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: event trigger - test synthetic_events syntax parser
> +
> +do_reset() {
> +    reset_trigger
> +    echo > set_event
> +    clear_trace
> +}
> +
> +fail() { #msg
> +    do_reset
> +    echo $1
> +    exit_fail
> +}
> +
> +if [ ! -f set_event ]; then
> +    echo "event tracing is not supported"
> +    exit_unsupported
> +fi
> +
> +if [ ! -f synthetic_events ]; then
> +    echo "synthetic event is not supported"
> +    exit_unsupported
> +fi
> +
> +reset_tracer
> +do_reset
> +
> +echo "Test synthetic_events syntax parser"
> +
> +echo > synthetic_events
> +
> +# synthetic event must have a field
> +! echo "myevent" >> synthetic_events
> +echo "myevent u64 var1" >> synthetic_events
> +
> +# synthetic event must be found in synthetic_events
> +grep "myevent[[:space:]]u64 var1" synthetic_events
> +
> +# it is not possible to add same name event
> +! echo "myevent u64 var2" >> synthetic_events
> +
> +# Non-append open will cleanup all events and add new one
> +echo "myevent u64 var2" > synthetic_events
> +
> +# multiple fields with different spaces
> +echo "myevent u64 var1; u64 var2;" > synthetic_events
> +grep "myevent[[:space:]]u64 var1; u64 var2" synthetic_events
> +echo "myevent u64 var1 ; u64 var2 ;" > synthetic_events
> +grep "myevent[[:space:]]u64 var1; u64 var2" synthetic_events
> +echo "myevent u64 var1 ;u64 var2" > synthetic_events
> +grep "myevent[[:space:]]u64 var1; u64 var2" synthetic_events
> +
> +# test field types
> +echo "myevent u32 var" > synthetic_events
> +echo "myevent u16 var" > synthetic_events
> +echo "myevent u8 var" > synthetic_events
> +echo "myevent s64 var" > synthetic_events
> +echo "myevent s32 var" > synthetic_events
> +echo "myevent s16 var" > synthetic_events
> +echo "myevent s8 var" > synthetic_events
> +
> +echo "myevent char var" > synthetic_events
> +echo "myevent int var" > synthetic_events
> +echo "myevent long var" > synthetic_events
> +echo "myevent pid_t var" > synthetic_events
> +
> +echo "myevent unsigned char var" > synthetic_events
> +echo "myevent unsigned int var" > synthetic_events
> +echo "myevent unsigned long var" > synthetic_events
> +grep "myevent[[:space:]]unsigned long var" synthetic_events
> +
> +# test string type
> +echo "myevent char var[10]" > synthetic_events
> +grep "myevent[[:space:]]char\[10\] var" synthetic_events
> +
> +do_reset
> +
> +exit 0

Re: [PATCH v2 3/3] selftests: ftrace: Add synthetic event syntax testcase

[Shuah Khan]

On 10/18/2018 02:34 PM, Steven Rostedt wrote:
> 
> Hi Masami,
> 
> Thanks! I pulled this into my urgent queue and will be running tests on
> it over night.
> 
> Shuah, can you ack this patch, as I want to add it to this release,
> along with the other changes.
> 

Acked-by: Shuah Khan <shuah@kernel.org>

thanks,
-- Shuah

Re: [PATCH v2 3/3] selftests: ftrace: Add synthetic event syntax testcase

[Steven Rostedt]

On Thu, 18 Oct 2018 14:38:22 -0600
Shuah Khan <shuah@kernel.org> wrote:

> On 10/18/2018 02:34 PM, Steven Rostedt wrote:
> > 
> > Hi Masami,
> > 
> > Thanks! I pulled this into my urgent queue and will be running tests on
> > it over night.
> > 
> > Shuah, can you ack this patch, as I want to add it to this release,
> > along with the other changes.
> >   
> 
> Acked-by: Shuah Khan <shuah@kernel.org>
> 

Thanks for the quick response!

-- Steve

Re: [PATCH v2 3/3] selftests: ftrace: Add synthetic event syntax testcase

[Masami Hiramatsu]

On Thu, 18 Oct 2018 16:34:27 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Hi Masami,
> 
> Thanks! I pulled this into my urgent queue and will be running tests on
> it over night.

Thanks!
I hope to pass the test :)

> Shuah, can you ack this patch, as I want to add it to this release,
> along with the other changes.

Shuah, just a note that this test case must fail unless these fixes are applied.

Thank you,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 3/3] selftests: ftrace: Add synthetic event syntax testcase

[Steven Rostedt]

On Fri, 19 Oct 2018 09:38:18 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > Shuah, can you ack this patch, as I want to add it to this release,
> > along with the other changes.  
> 
> Shuah, just a note that this test case must fail unless these fixes are applied.

This is why I wanted to attache them to my pull request :-)

-- Steve

Re: [PATCH v2 0/3] tracing: Fix synthetic event parser

[Tom Zanussi]

Hi Masami,

Sorry for the late reply on this - was out on vacation last week.

On Thu, 2018-10-18 at 22:11 +0900, Masami Hiramatsu wrote:
> Hi,
> 
> Here are 2 fixes for synthetic event parser and its testcase.
> One bug is that the synthetic_events can not accept "unsigned"
> modifier for the field type, and another one is not allowing
> the last independent semicolon.
> 
> I found that there is no testcase for checking it too. So the
> last patch in this series adding a testcase for synthetic
> event parser.
> 

These patches look good to me, so for the lot:

Acked-by: Tom Zanussi <zanussi@linux.intel.com>
Tested-by: Tom Zanussi <zanussi@linux.intel.com>

Thanks,

Tom


> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       tracing: Fix synthetic event to accept unsigned modifier
>       tracing: Fix synthetic event to allow semicolon at end
>       selftests: ftrace: Add synthetic event syntax testcase
> 
> 
>  kernel/trace/trace_events_hist.c                   |   32 ++++++--
>  .../inter-event/trigger-synthetic-event-syntax.tc  |   80
> ++++++++++++++++++++
>  2 files changed, 105 insertions(+), 7 deletions(-)
>  create mode 100644
> tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-
> synthetic-event-syntax.tc
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>