[PATCH 0/4] tracing: More synthetic event error fixes

[PATCH 0/4] tracing: More synthetic event error fixes

[Tom Zanussi]

Hi,

This patchset addresses the synthetic event error anomalies reported
by Masami in the last patchset [1].

It turns out that most of the problems boil down to clunky separator
parsing; adding a couple new abilities to trace_run_command() and then
adapting the existing users seemed to me the best way to fix these
things, and also gets rid of some code.

Also, to make things easier for error display, I changed these to
preserve the original command string and pass it through the callback
instead of rebuilding it for error display.

I added some new error strings and removed unused ones as well, and
added a bunch of new test cases to the synthetic parser error test
case.

I didn't see any problems running the entire ftrace testsuite or the
test modules that also use the things that were touched here.

Thanks,

Tom

[1] https://lore.kernel.org/lkml/20201014110636.139df7be275d40a23b523b84@kernel.org/

The following changes since commit 6107742d15832011cd0396d821f3225b52551f1f:

  tracing: support "bool" type in synthetic trace events (2020-10-15 12:01:14 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/synth-fixes-v1

Tom Zanussi (4):
  tracing: Make trace_*_run_command() more flexible
  tracing: Use new trace_run_command() options
  tracing: Update synth command errors
  selftests/ftrace: Update synthetic event syntax errors

 kernel/trace/trace.c                          | 41 ++++++++--
 kernel/trace/trace.h                          | 12 ++-
 kernel/trace/trace_dynevent.c                 |  4 +-
 kernel/trace/trace_events_synth.c             | 79 ++++---------------
 kernel/trace/trace_kprobe.c                   |  5 +-
 kernel/trace/trace_uprobe.c                   |  5 +-
 .../trigger-synthetic_event_syntax_errors.tc  | 17 ++--
 7 files changed, 78 insertions(+), 85 deletions(-)

-- 
2.17.1

[PATCH 1/4] tracing: Make trace_*_run_command() more flexible

[Tom Zanussi]

trace_run_command() and therefore functions that use it, such as
trace_parse_run_command(), uses argv_split() to split the command into
an array of args then passed to the callback to handle.

This works fine for most commands but some commands would like to
allow the user to use and additional separator to visually group items
in the command.  One example would be synthetic event commands, which
use semicolons to group fields:

  echo 'event_name int a; char b[]; u64 lat' >> synthetic_events

What gets passed as an args array to the command for a command like
this include tokens that have semicolons included with the token,
which the callback then needs to strip out, since argv_split() only
looks at whitespace as a separator.

It would be nicer to just have trace_run_command() strip out the
semicolons at its level rather than passing that task onto the
callback.  To accomplish that, this change adds an 'additional_sep'
arg to a new __trace_run_command() function that allows a caller to
pass an additional separator char that if non-zero simply replaces
that character with whitespace before splitting the command into args.
The original trace_run_command() remains as-is in this regard, simply
calling __trace_run_command() with 0 for the separator, while making a
new trace_run_command_add_sep() version that can be used to pass in a
separator.

The other change here simply has __trace_run_command() make a copy of
the original command to work on, while leaving the original command
string untouched.  This untouched string is now passed into the
callback as well - this allows the callback to use the original string
for error processing and caret placement rather than forcing the
callback to recreate the original string from the args, which isn't
always possible.

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace.c              | 41 +++++++++++++++++++++++++------
 kernel/trace/trace.h              | 12 ++++++---
 kernel/trace/trace_dynevent.c     |  4 +--
 kernel/trace/trace_events_synth.c |  5 ++--
 kernel/trace/trace_kprobe.c       |  5 ++--
 kernel/trace/trace_uprobe.c       |  5 ++--
 6 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 63c97012ed39..afa526414b25 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9367,30 +9367,56 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 }
 EXPORT_SYMBOL_GPL(ftrace_dump);
 
-int trace_run_command(const char *buf, int (*createfn)(int, char **))
+static int __trace_run_command(const char *buf,
+			       int (*createfn)(int, char **, const char *),
+			       char additional_sep)
 {
-	char **argv;
 	int argc, ret;
+	char **argv, *cmd;
+
+	cmd = kstrdup(buf, GFP_KERNEL);
+	if (!cmd)
+		return -ENOMEM;
+
+	if (additional_sep)
+		strreplace(cmd, additional_sep, ' ');
 
 	argc = 0;
 	ret = 0;
-	argv = argv_split(GFP_KERNEL, buf, &argc);
-	if (!argv)
+	argv = argv_split(GFP_KERNEL, cmd, &argc);
+	if (!argv) {
+		kfree(cmd);
 		return -ENOMEM;
+	}
 
 	if (argc)
-		ret = createfn(argc, argv);
+		ret = createfn(argc, argv, buf);
 
 	argv_free(argv);
+	kfree(cmd);
 
 	return ret;
 }
 
+int trace_run_command(const char *buf, int (*createfn)(int, char **,
+						       const char *))
+{
+	return __trace_run_command(buf, createfn, 0);
+}
+
+int trace_run_command_add_sep(const char *buf,
+			      int (*createfn)(int, char **, const char *),
+			      char additional_sep)
+{
+	return __trace_run_command(buf, createfn, additional_sep);
+}
+
 #define WRITE_BUFSIZE  4096
 
 ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
 				size_t count, loff_t *ppos,
-				int (*createfn)(int, char **))
+				int (*createfn)(int, char **, const char *),
+				char additional_sep)
 {
 	char *kbuf, *buf, *tmp;
 	int ret = 0;
@@ -9438,7 +9464,8 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
 			if (tmp)
 				*tmp = '\0';
 
-			ret = trace_run_command(buf, createfn);
+			ret = trace_run_command_add_sep(buf, createfn,
+							additional_sep);
 			if (ret)
 				goto out;
 			buf += size;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 34e0c4d5a6e7..ae6e0c2ec028 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1982,10 +1982,16 @@ extern int tracing_set_cpumask(struct trace_array *tr,
 
 #define MAX_EVENT_NAME_LEN	64
 
-extern int trace_run_command(const char *buf, int (*createfn)(int, char**));
+extern int trace_run_command(const char *buf,
+			     int (*createfn)(int, char **, const char *));
+extern int trace_run_command_add_sep(const char *buf,
+				     int (*createfn)(int, char **, const char *),
+				     char additional_sep);
 extern ssize_t trace_parse_run_command(struct file *file,
-		const char __user *buffer, size_t count, loff_t *ppos,
-		int (*createfn)(int, char**));
+				       const char __user *buffer,
+				       size_t count, loff_t *ppos,
+				       int (*createfn)(int, char **, const char *),
+				       char additional_sep);
 
 extern unsigned int err_pos(char *cmd, const char *str);
 extern void tracing_log_err(struct trace_array *tr,
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index 5fa49cfd2bb6..4dc21c1879ae 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -75,7 +75,7 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
 	return ret;
 }
 
-static int create_dyn_event(int argc, char **argv)
+static int create_dyn_event(int argc, char **argv, const char *raw_cmd)
 {
 	struct dyn_event_operations *ops;
 	int ret = -ENODEV;
@@ -191,7 +191,7 @@ static ssize_t dyn_event_write(struct file *file, const char __user *buffer,
 				size_t count, loff_t *ppos)
 {
 	return trace_parse_run_command(file, buffer, count, ppos,
-				       create_dyn_event);
+				       create_dyn_event, ';');
 }
 
 static const struct file_operations dynamic_events_ops = {
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 3212e2c653b3..e6659bb9a500 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -1378,7 +1378,8 @@ int synth_event_delete(const char *event_name)
 }
 EXPORT_SYMBOL_GPL(synth_event_delete);
 
-static int create_or_delete_synth_event(int argc, char **argv)
+static int create_or_delete_synth_event(int argc, char **argv,
+					const char *raw_cmd)
 {
 	const char *name = argv[0];
 	int ret;
@@ -2046,7 +2047,7 @@ static ssize_t synth_events_write(struct file *file,
 				  size_t count, loff_t *ppos)
 {
 	return trace_parse_run_command(file, buffer, count, ppos,
-				       create_or_delete_synth_event);
+				       create_or_delete_synth_event, ';');
 }
 
 static const struct file_operations synth_events_fops = {
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index b911e9f6d9f5..54af5ff58923 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -907,7 +907,8 @@ static int trace_kprobe_create(int argc, const char *argv[])
 	goto out;
 }
 
-static int create_or_delete_trace_kprobe(int argc, char **argv)
+static int create_or_delete_trace_kprobe(int argc, char **argv,
+					 const char *raw_cmd)
 {
 	int ret;
 
@@ -1159,7 +1160,7 @@ static ssize_t probes_write(struct file *file, const char __user *buffer,
 			    size_t count, loff_t *ppos)
 {
 	return trace_parse_run_command(file, buffer, count, ppos,
-				       create_or_delete_trace_kprobe);
+				       create_or_delete_trace_kprobe, 0);
 }
 
 static const struct file_operations kprobe_events_ops = {
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3cf7128e1ad3..b2840003b851 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -716,7 +716,8 @@ static int trace_uprobe_create(int argc, const char **argv)
 	return ret;
 }
 
-static int create_or_delete_trace_uprobe(int argc, char **argv)
+static int create_or_delete_trace_uprobe(int argc, char **argv,
+					 const char *raw_cmd)
 {
 	int ret;
 
@@ -793,7 +794,7 @@ static ssize_t probes_write(struct file *file, const char __user *buffer,
 			    size_t count, loff_t *ppos)
 {
 	return trace_parse_run_command(file, buffer, count, ppos,
-					create_or_delete_trace_uprobe);
+				       create_or_delete_trace_uprobe, 0);
 }
 
 static const struct file_operations uprobe_events_ops = {
-- 
2.17.1

[PATCH 2/4] tracing: Use new trace_run_command() options

[Tom Zanussi]

Now that the synthetic event command passes in ';' as an additional
separator to trace_parse_run_command(), all of the parsing related to
semicolon can be removed.

Additionally, the since create_or_delete_synth_event() now receives
the raw command string, it doesn't need save_cmdstr(), which can also
be removed.

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace_events_synth.c | 65 ++++---------------------------
 1 file changed, 8 insertions(+), 57 deletions(-)

diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index e6659bb9a500..2333025ef31e 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -48,7 +48,7 @@ static int errpos(const char *str)
 	return err_pos(last_cmd, str);
 }
 
-static void last_cmd_set(char *str)
+static void last_cmd_set(const char *str)
 {
 	if (!str)
 		return;
@@ -587,9 +587,6 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	int len, ret = 0;
 	ssize_t size;
 
-	if (field_type[0] == ';')
-		field_type++;
-
 	if (!strcmp(field_type, "unsigned")) {
 		if (argc < 3) {
 			synth_err(SYNTH_ERR_INCOMPLETE_TYPE, errpos(field_type));
@@ -612,8 +609,6 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	array = strchr(field_name, '[');
 	if (array)
 		len -= strlen(array);
-	else if (field_name[len - 1] == ';')
-		len--;
 
 	field->name = kmemdup_nul(field_name, len, GFP_KERNEL);
 	if (!field->name) {
@@ -626,17 +621,11 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 		goto free;
 	}
 
-	if (field_type[0] == ';')
-		field_type++;
 	len = strlen(field_type) + 1;
 
-        if (array) {
-                int l = strlen(array);
+        if (array)
+                len += strlen(array);
 
-                if (l && array[l - 1] == ';')
-                        l--;
-                len += l;
-        }
 	if (prefix)
 		len += strlen(prefix);
 
@@ -648,11 +637,8 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	if (prefix)
 		strcat(field->type, prefix);
 	strcat(field->type, field_type);
-	if (array) {
+	if (array)
 		strcat(field->type, array);
-		if (field->type[len - 1] == ';')
-			field->type[len - 1] = '\0';
-	}
 
 	size = synth_field_size(field->type);
 	if (size < 0) {
@@ -1155,47 +1141,12 @@ int synth_event_gen_cmd_array_start(struct dynevent_cmd *cmd, const char *name,
 }
 EXPORT_SYMBOL_GPL(synth_event_gen_cmd_array_start);
 
-static int save_cmdstr(int argc, const char *name, const char **argv)
-{
-	struct seq_buf s;
-	char *buf;
-	int i;
-
-	buf = kzalloc(MAX_DYNEVENT_CMD_LEN, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	seq_buf_init(&s, buf, MAX_DYNEVENT_CMD_LEN);
-
-	seq_buf_puts(&s, name);
-
-	for (i = 0; i < argc; i++) {
-		seq_buf_putc(&s, ' ');
-		seq_buf_puts(&s, argv[i]);
-	}
-
-	if (!seq_buf_buffer_left(&s)) {
-		synth_err(SYNTH_ERR_CMD_TOO_LONG, 0);
-		kfree(buf);
-		return -EINVAL;
-	}
-	buf[s.len] = 0;
-	last_cmd_set(buf);
-
-	kfree(buf);
-	return 0;
-}
-
 static int __create_synth_event(int argc, const char *name, const char **argv)
 {
 	struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
 	struct synth_event *event = NULL;
 	int i, consumed = 0, n_fields = 0, ret = 0;
 
-	ret = save_cmdstr(argc, name, argv);
-	if (ret)
-		return ret;
-
 	/*
 	 * Argument syntax:
 	 *  - Add synthetic event: <event_name> field[;field] ...
@@ -1224,8 +1175,6 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
 	}
 
 	for (i = 0; i < argc - 1; i++) {
-		if (strcmp(argv[i], ";") == 0)
-			continue;
 		if (n_fields == SYNTH_FIELDS_MAX) {
 			synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
 			ret = -EINVAL;
@@ -1241,7 +1190,7 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
 		i += consumed - 1;
 	}
 
-	if (i < argc && strcmp(argv[i], ";") != 0) {
+	if (i < argc) {
 		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(argv[i]));
 		ret = -EINVAL;
 		goto err;
@@ -1384,6 +1333,8 @@ static int create_or_delete_synth_event(int argc, char **argv,
 	const char *name = argv[0];
 	int ret;
 
+	last_cmd_set(raw_cmd);
+
 	/* trace_run_command() ensures argc != 0 */
 	if (name[0] == '!') {
 		ret = synth_event_delete(name + 1);
@@ -1399,7 +1350,7 @@ static int synth_event_run_command(struct dynevent_cmd *cmd)
 	struct synth_event *se;
 	int ret;
 
-	ret = trace_run_command(cmd->seq.buffer, create_or_delete_synth_event);
+	ret = trace_run_command_add_sep(cmd->seq.buffer, create_or_delete_synth_event, ';');
 	if (ret)
 		return ret;
 
-- 
2.17.1

[PATCH 3/4] tracing: Update synth command errors

[Tom Zanussi]

Since array types are handled differently, errors referencing them
also need to be handled differently.  Add and use a new
INVALID_ARRAY_SPEC error.  Also remove INVALID_FIELD since it can
better be handled using CMD_INCOMPLETE, and remove CMD_TOO_LONG, since
it's no longer used.

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace_events_synth.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 2333025ef31e..f48964bdd66e 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -28,8 +28,7 @@
 	C(TOO_MANY_FIELDS,	"Too many fields"),		\
 	C(INCOMPLETE_TYPE,	"Incomplete type"),		\
 	C(INVALID_TYPE,		"Invalid type"),		\
-	C(INVALID_FIELD,	"Invalid field"),		\
-	C(CMD_TOO_LONG,		"Command too long"),
+	C(INVALID_ARRAY_SPEC,	"Invalid array specification"),
 
 #undef C
 #define C(a, b)		SYNTH_ERR_##a
@@ -643,6 +642,10 @@ static struct synth_field *parse_synth_field(int argc, const char **argv,
 	size = synth_field_size(field->type);
 	if (size < 0) {
 		synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
+		if (array)
+			synth_err(SYNTH_ERR_INVALID_ARRAY_SPEC, errpos(field_name));
+		else
+			synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
 		ret = -EINVAL;
 		goto free;
 	} else if (size == 0) {
@@ -1191,7 +1194,7 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
 	}
 
 	if (i < argc) {
-		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(argv[i]));
+		synth_err(SYNTH_ERR_CMD_INCOMPLETE, errpos(argv[i]));
 		ret = -EINVAL;
 		goto err;
 	}
-- 
2.17.1

[PATCH 4/4] selftests/ftrace: Update synthetic event syntax errors

[Tom Zanussi]

Some of the synthetic event errors and positions have changed in the
code - update those and add several more tests.

Also add a runtime check to ensure that the kernel supports dynamic
strings in synthetic events, which these tests require.

Fixes: 81ff92a93d95 (selftests/ftrace: Add test case for synthetic
event syntax errors)

Reported-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 .../trigger-synthetic_event_syntax_errors.tc    | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
index ada594fe16cb..ebdc7c0a75a1 100644
--- a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-synthetic_event_syntax_errors.tc
@@ -1,19 +1,22 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
 # description: event trigger - test synthetic_events syntax parser errors
-# requires: synthetic_events error_log
+# requires: synthetic_events error_log "char name[]' >> synthetic_events":README
 
 check_error() { # command-with-error-pos-by-^
     ftrace_errlog_check 'synthetic_events' "$1" 'synthetic_events'
 }
 
 check_error 'myevent ^chr arg'			# INVALID_TYPE
-check_error 'myevent ^char str[];; int v'	# INVALID_TYPE
-check_error 'myevent char ^str]; int v'		# INVALID_NAME
-check_error 'myevent char ^str;[]'		# INVALID_NAME
-check_error 'myevent ^char str[; int v'		# INVALID_TYPE
-check_error '^mye;vent char str[]'		# BAD_NAME
-check_error 'myevent char str[]; ^int'		# INVALID_FIELD
+check_error 'mye;^vent char str[]'		# INVALID_TYPE
+check_error 'myevent char ^str]; int v'		# BAD_NAME
+check_error '^mye-vent char str[]'		# BAD_NAME
+check_error 'myevent char ^st-r[]'		# BAD_NAME
+check_error 'myevent char str;^[]'		# INCOMPLETE_CMD
+check_error 'myevent char str[]; ^int'		# INCOMPLETE_CMD
 check_error '^myevent'				# INCOMPLETE_CMD
+check_error 'myevent char ^str[; int v'		# INVALID_ARRAY_SPEC
+check_error 'myevent char ^str[kdjdk]'		# INVALID_ARRAY_SPEC
+check_error 'myevent char ^str[257]'		# INVALID_ARRAY_SPEC
 
 exit 0
-- 
2.17.1

Re: [PATCH 1/4] tracing: Make trace_*_run_command() more flexible

[Masami Hiramatsu]

Hi Tom,

On Fri, 16 Oct 2020 16:48:22 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> trace_run_command() and therefore functions that use it, such as
> trace_parse_run_command(), uses argv_split() to split the command into
> an array of args then passed to the callback to handle.
> 
> This works fine for most commands but some commands would like to
> allow the user to use and additional separator to visually group items
> in the command.  One example would be synthetic event commands, which
> use semicolons to group fields:
> 
>   echo 'event_name int a; char b[]; u64 lat' >> synthetic_events
> 
> What gets passed as an args array to the command for a command like
> this include tokens that have semicolons included with the token,
> which the callback then needs to strip out, since argv_split() only
> looks at whitespace as a separator.
> 
> It would be nicer to just have trace_run_command() strip out the
> semicolons at its level rather than passing that task onto the
> callback. To accomplish that, this change adds an 'additional_sep'
> arg to a new __trace_run_command() function that allows a caller to
> pass an additional separator char that if non-zero simply replaces
> that character with whitespace before splitting the command into args.
> The original trace_run_command() remains as-is in this regard, simply
> calling __trace_run_command() with 0 for the separator, while making a
> new trace_run_command_add_sep() version that can be used to pass in a
> separator.

No, I don't like to tweak trace_run_command() that way, I'm OK to
delegate the argv_split() totally to the callback function (pass
the raw command string to the callback), OR __create_synth_event()
concatinate the fields argv and parse it by itself (I think the
latter is better and simpler).

The ";" separator is not an additinal separator but that is higher
level separator for synthetic event. Suppose that you get the
following input;
 "myevent int c ; char b"
 "myevent int;c;char;b;"
These should not be same for the synthetic events. The fields must be
splitted by ';' at first, and then each field should be parsed.

So, I recommend you not to pass the additional separator to the
generic function, but (since it is only for synthetic event) to
reconstruct the raw command from argv, and parse it again with
";" inside synthetic event parser. Then each fields parser can
check that is a wrong input or not.

It will be something like

__create_synth_event(argc, argv)
{
	<event-name parsing>
	argc--; argv++;

	raw_fields = concat_argv(argc, argv);// you can assume argv is generated by argv_split().
	vfields = split_fields(raw_fields, &nfields);// similar to argv_split()
	for (i = 0; i < nfields; i++)
		parse_synth_field(vfields[i]);
}

Then, you don't need to change the generic functions, and also
you will get the correct parser routines.

Thank you,

> 
> The other change here simply has __trace_run_command() make a copy of
> the original command to work on, while leaving the original command
> string untouched.  This untouched string is now passed into the
> callback as well - this allows the callback to use the original string
> for error processing and caret placement rather than forcing the
> callback to recreate the original string from the args, which isn't
> always possible.
> 
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> ---
>  kernel/trace/trace.c              | 41 +++++++++++++++++++++++++------
>  kernel/trace/trace.h              | 12 ++++++---
>  kernel/trace/trace_dynevent.c     |  4 +--
>  kernel/trace/trace_events_synth.c |  5 ++--
>  kernel/trace/trace_kprobe.c       |  5 ++--
>  kernel/trace/trace_uprobe.c       |  5 ++--
>  6 files changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 63c97012ed39..afa526414b25 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -9367,30 +9367,56 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
>  }
>  EXPORT_SYMBOL_GPL(ftrace_dump);
>  
> -int trace_run_command(const char *buf, int (*createfn)(int, char **))
> +static int __trace_run_command(const char *buf,
> +			       int (*createfn)(int, char **, const char *),
> +			       char additional_sep)
>  {
> -	char **argv;
>  	int argc, ret;
> +	char **argv, *cmd;
> +
> +	cmd = kstrdup(buf, GFP_KERNEL);
> +	if (!cmd)
> +		return -ENOMEM;
> +
> +	if (additional_sep)
> +		strreplace(cmd, additional_sep, ' ');
>  
>  	argc = 0;
>  	ret = 0;
> -	argv = argv_split(GFP_KERNEL, buf, &argc);
> -	if (!argv)
> +	argv = argv_split(GFP_KERNEL, cmd, &argc);
> +	if (!argv) {
> +		kfree(cmd);
>  		return -ENOMEM;
> +	}
>  
>  	if (argc)
> -		ret = createfn(argc, argv);
> +		ret = createfn(argc, argv, buf);
>  
>  	argv_free(argv);
> +	kfree(cmd);
>  
>  	return ret;
>  }
>  
> +int trace_run_command(const char *buf, int (*createfn)(int, char **,
> +						       const char *))
> +{
> +	return __trace_run_command(buf, createfn, 0);
> +}
> +
> +int trace_run_command_add_sep(const char *buf,
> +			      int (*createfn)(int, char **, const char *),
> +			      char additional_sep)
> +{
> +	return __trace_run_command(buf, createfn, additional_sep);
> +}
> +
>  #define WRITE_BUFSIZE  4096
>  
>  ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
>  				size_t count, loff_t *ppos,
> -				int (*createfn)(int, char **))
> +				int (*createfn)(int, char **, const char *),
> +				char additional_sep)
>  {
>  	char *kbuf, *buf, *tmp;
>  	int ret = 0;
> @@ -9438,7 +9464,8 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
>  			if (tmp)
>  				*tmp = '\0';
>  
> -			ret = trace_run_command(buf, createfn);
> +			ret = trace_run_command_add_sep(buf, createfn,
> +							additional_sep);
>  			if (ret)
>  				goto out;
>  			buf += size;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 34e0c4d5a6e7..ae6e0c2ec028 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1982,10 +1982,16 @@ extern int tracing_set_cpumask(struct trace_array *tr,
>  
>  #define MAX_EVENT_NAME_LEN	64
>  
> -extern int trace_run_command(const char *buf, int (*createfn)(int, char**));
> +extern int trace_run_command(const char *buf,
> +			     int (*createfn)(int, char **, const char *));
> +extern int trace_run_command_add_sep(const char *buf,
> +				     int (*createfn)(int, char **, const char *),
> +				     char additional_sep);
>  extern ssize_t trace_parse_run_command(struct file *file,
> -		const char __user *buffer, size_t count, loff_t *ppos,
> -		int (*createfn)(int, char**));
> +				       const char __user *buffer,
> +				       size_t count, loff_t *ppos,
> +				       int (*createfn)(int, char **, const char *),
> +				       char additional_sep);
>  
>  extern unsigned int err_pos(char *cmd, const char *str);
>  extern void tracing_log_err(struct trace_array *tr,
> diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
> index 5fa49cfd2bb6..4dc21c1879ae 100644
> --- a/kernel/trace/trace_dynevent.c
> +++ b/kernel/trace/trace_dynevent.c
> @@ -75,7 +75,7 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
>  	return ret;
>  }
>  
> -static int create_dyn_event(int argc, char **argv)
> +static int create_dyn_event(int argc, char **argv, const char *raw_cmd)
>  {
>  	struct dyn_event_operations *ops;
>  	int ret = -ENODEV;
> @@ -191,7 +191,7 @@ static ssize_t dyn_event_write(struct file *file, const char __user *buffer,
>  				size_t count, loff_t *ppos)
>  {
>  	return trace_parse_run_command(file, buffer, count, ppos,
> -				       create_dyn_event);
> +				       create_dyn_event, ';');
>  }
>  
>  static const struct file_operations dynamic_events_ops = {
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index 3212e2c653b3..e6659bb9a500 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -1378,7 +1378,8 @@ int synth_event_delete(const char *event_name)
>  }
>  EXPORT_SYMBOL_GPL(synth_event_delete);
>  
> -static int create_or_delete_synth_event(int argc, char **argv)
> +static int create_or_delete_synth_event(int argc, char **argv,
> +					const char *raw_cmd)
>  {
>  	const char *name = argv[0];
>  	int ret;
> @@ -2046,7 +2047,7 @@ static ssize_t synth_events_write(struct file *file,
>  				  size_t count, loff_t *ppos)
>  {
>  	return trace_parse_run_command(file, buffer, count, ppos,
> -				       create_or_delete_synth_event);
> +				       create_or_delete_synth_event, ';');
>  }
>  
>  static const struct file_operations synth_events_fops = {
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index b911e9f6d9f5..54af5ff58923 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -907,7 +907,8 @@ static int trace_kprobe_create(int argc, const char *argv[])
>  	goto out;
>  }
>  
> -static int create_or_delete_trace_kprobe(int argc, char **argv)
> +static int create_or_delete_trace_kprobe(int argc, char **argv,
> +					 const char *raw_cmd)
>  {
>  	int ret;
>  
> @@ -1159,7 +1160,7 @@ static ssize_t probes_write(struct file *file, const char __user *buffer,
>  			    size_t count, loff_t *ppos)
>  {
>  	return trace_parse_run_command(file, buffer, count, ppos,
> -				       create_or_delete_trace_kprobe);
> +				       create_or_delete_trace_kprobe, 0);
>  }
>  
>  static const struct file_operations kprobe_events_ops = {
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 3cf7128e1ad3..b2840003b851 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -716,7 +716,8 @@ static int trace_uprobe_create(int argc, const char **argv)
>  	return ret;
>  }
>  
> -static int create_or_delete_trace_uprobe(int argc, char **argv)
> +static int create_or_delete_trace_uprobe(int argc, char **argv,
> +					 const char *raw_cmd)
>  {
>  	int ret;
>  
> @@ -793,7 +794,7 @@ static ssize_t probes_write(struct file *file, const char __user *buffer,
>  			    size_t count, loff_t *ppos)
>  {
>  	return trace_parse_run_command(file, buffer, count, ppos,
> -					create_or_delete_trace_uprobe);
> +				       create_or_delete_trace_uprobe, 0);
>  }
>  
>  static const struct file_operations uprobe_events_ops = {
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/4] tracing: More synthetic event error fixes

[Masami Hiramatsu]

Hi Tom,

On Fri, 16 Oct 2020 16:48:21 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> Hi,
> 
> This patchset addresses the synthetic event error anomalies reported
> by Masami in the last patchset [1].
> 
> It turns out that most of the problems boil down to clunky separator
> parsing; adding a couple new abilities to trace_run_command() and then
> adapting the existing users seemed to me the best way to fix these
> things, and also gets rid of some code.

Thanks for your work! But I think the interface design is a bit add-hoc.
I sent a comment mail on [1/4]. Let's discuss on the thread.

> Also, to make things easier for error display, I changed these to
> preserve the original command string and pass it through the callback
> instead of rebuilding it for error display.

trace_*probe reconstruct the input from argv, are there any reason
synthetic event can not do the same thing?
(this means the command on error message can be a bit different,
 see __trace_probe_log_err() in kernel/trace/trace_probe.c)

> 
> I added some new error strings and removed unused ones as well, and
> added a bunch of new test cases to the synthetic parser error test
> case.

Good.

> 
> I didn't see any problems running the entire ftrace testsuite or the
> test modules that also use the things that were touched here.

Thanks,

> 
> Thanks,
> 
> Tom
> 
> [1] https://lore.kernel.org/lkml/20201014110636.139df7be275d40a23b523b84@kernel.org/
> 
> The following changes since commit 6107742d15832011cd0396d821f3225b52551f1f:
> 
>   tracing: support "bool" type in synthetic trace events (2020-10-15 12:01:14 -0400)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/synth-fixes-v1
> 
> Tom Zanussi (4):
>   tracing: Make trace_*_run_command() more flexible
>   tracing: Use new trace_run_command() options
>   tracing: Update synth command errors
>   selftests/ftrace: Update synthetic event syntax errors
> 
>  kernel/trace/trace.c                          | 41 ++++++++--
>  kernel/trace/trace.h                          | 12 ++-
>  kernel/trace/trace_dynevent.c                 |  4 +-
>  kernel/trace/trace_events_synth.c             | 79 ++++---------------
>  kernel/trace/trace_kprobe.c                   |  5 +-
>  kernel/trace/trace_uprobe.c                   |  5 +-
>  .../trigger-synthetic_event_syntax_errors.tc  | 17 ++--
>  7 files changed, 78 insertions(+), 85 deletions(-)
> 
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/4] tracing: Make trace_*_run_command() more flexible

[Masami Hiramatsu]

Hi Tom,

On Sun, 18 Oct 2020 23:20:11 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Tom,
> 
> On Fri, 16 Oct 2020 16:48:22 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > trace_run_command() and therefore functions that use it, such as
> > trace_parse_run_command(), uses argv_split() to split the command into
> > an array of args then passed to the callback to handle.
> > 
> > This works fine for most commands but some commands would like to
> > allow the user to use and additional separator to visually group items
> > in the command.  One example would be synthetic event commands, which
> > use semicolons to group fields:
> > 
> >   echo 'event_name int a; char b[]; u64 lat' >> synthetic_events
> > 
> > What gets passed as an args array to the command for a command like
> > this include tokens that have semicolons included with the token,
> > which the callback then needs to strip out, since argv_split() only
> > looks at whitespace as a separator.
> > 
> > It would be nicer to just have trace_run_command() strip out the
> > semicolons at its level rather than passing that task onto the
> > callback. To accomplish that, this change adds an 'additional_sep'
> > arg to a new __trace_run_command() function that allows a caller to
> > pass an additional separator char that if non-zero simply replaces
> > that character with whitespace before splitting the command into args.
> > The original trace_run_command() remains as-is in this regard, simply
> > calling __trace_run_command() with 0 for the separator, while making a
> > new trace_run_command_add_sep() version that can be used to pass in a
> > separator.
> 
> No, I don't like to tweak trace_run_command() that way, I'm OK to
> delegate the argv_split() totally to the callback function (pass
> the raw command string to the callback), OR __create_synth_event()
> concatinate the fields argv and parse it by itself (I think the
> latter is better and simpler).
> 
> The ";" separator is not an additinal separator but that is higher
> level separator for synthetic event. Suppose that you get the
> following input;
>  "myevent int c ; char b"
>  "myevent int;c;char;b;"
> These should not be same for the synthetic events. The fields must be
> splitted by ';' at first, and then each field should be parsed.
> 
> So, I recommend you not to pass the additional separator to the
> generic function, but (since it is only for synthetic event) to
> reconstruct the raw command from argv, and parse it again with
> ";" inside synthetic event parser. Then each fields parser can
> check that is a wrong input or not.
> 
> It will be something like
> 
> __create_synth_event(argc, argv)
> {
> 	<event-name parsing>
> 	argc--; argv++;
> 
> 	raw_fields = concat_argv(argc, argv);// you can assume argv is generated by argv_split().
> 	vfields = split_fields(raw_fields, &nfields);// similar to argv_split()
> 	for (i = 0; i < nfields; i++)
> 		parse_synth_field(vfields[i]);
> }
> 
> Then, you don't need to change the generic functions, and also
> you will get the correct parser routines.

If you think the split->concat->split process is redundant, I think we
can remove trace_run_command() and just pass a raw command string to each
event command parser as I said above.

In this way, each event create function can parse the command by themselves.
So you can parse the command as you like.

Here is the patch which modifies trace-{k,u}probe and trace-dynevent
side, but not changing synthetic event side yet. Will this help you?

From 5aed03a8df0926d92f6585b3932fdc96324cb82d Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <mhiramat@kernel.org>
Date: Sat, 17 Oct 2020 23:11:29 +0900
Subject: [PATCH] tracing/dynevent: Delegate parsing to create function

Delegate command parsing to each create function so that the
command syntax can be customized.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace.c          | 23 ++---------------------
 kernel/trace/trace.h          |  3 +--
 kernel/trace/trace_dynevent.c | 35 ++++++++++++++++++++++-------------
 kernel/trace/trace_dynevent.h |  4 ++--
 kernel/trace/trace_kprobe.c   | 33 ++++++++++++++++++---------------
 kernel/trace/trace_probe.c    | 17 +++++++++++++++++
 kernel/trace/trace_probe.h    |  1 +
 kernel/trace/trace_uprobe.c   | 17 +++++++++++------
 8 files changed, 74 insertions(+), 59 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 63c97012ed39..277d97220971 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9367,30 +9367,11 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 }
 EXPORT_SYMBOL_GPL(ftrace_dump);
 
-int trace_run_command(const char *buf, int (*createfn)(int, char **))
-{
-	char **argv;
-	int argc, ret;
-
-	argc = 0;
-	ret = 0;
-	argv = argv_split(GFP_KERNEL, buf, &argc);
-	if (!argv)
-		return -ENOMEM;
-
-	if (argc)
-		ret = createfn(argc, argv);
-
-	argv_free(argv);
-
-	return ret;
-}
-
 #define WRITE_BUFSIZE  4096
 
 ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
 				size_t count, loff_t *ppos,
-				int (*createfn)(int, char **))
+				int (*createfn)(const char *))
 {
 	char *kbuf, *buf, *tmp;
 	int ret = 0;
@@ -9438,7 +9419,7 @@ ssize_t trace_parse_run_command(struct file *file, const char __user *buffer,
 			if (tmp)
 				*tmp = '\0';
 
-			ret = trace_run_command(buf, createfn);
+			ret = createfn(buf);
 			if (ret)
 				goto out;
 			buf += size;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 34e0c4d5a6e7..02d7c487a30b 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1982,10 +1982,9 @@ extern int tracing_set_cpumask(struct trace_array *tr,
 
 #define MAX_EVENT_NAME_LEN	64
 
-extern int trace_run_command(const char *buf, int (*createfn)(int, char**));
 extern ssize_t trace_parse_run_command(struct file *file,
 		const char __user *buffer, size_t count, loff_t *ppos,
-		int (*createfn)(int, char**));
+		int (*createfn)(const char *));
 
 extern unsigned int err_pos(char *cmd, const char *str);
 extern void tracing_log_err(struct trace_array *tr,
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index 5fa49cfd2bb6..af83bc5447fe 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -31,23 +31,31 @@ int dyn_event_register(struct dyn_event_operations *ops)
 	return 0;
 }
 
-int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
+int dyn_event_release(const char *raw_command, struct dyn_event_operations *type)
 {
 	struct dyn_event *pos, *n;
 	char *system = NULL, *event, *p;
-	int ret = -ENOENT;
+	int argc, ret = -ENOENT;
+	char **argv;
+
+	argv = argv_split(GFP_KERNEL, raw_command, &argc);
+	if (!argv)
+		return -ENOMEM;
 
 	if (argv[0][0] == '-') {
-		if (argv[0][1] != ':')
-			return -EINVAL;
+		if (argv[0][1] != ':') {
+			ret = -EINVAL;
+			goto out;
+		}
 		event = &argv[0][2];
 	} else {
 		event = strchr(argv[0], ':');
-		if (!event)
-			return -EINVAL;
+		if (!event) {
+			ret = -EINVAL;
+			goto out;
+		}
 		event++;
 	}
-	argc--; argv++;
 
 	p = strchr(event, '/');
 	if (p) {
@@ -63,7 +71,7 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
 		if (type && type != pos->ops)
 			continue;
 		if (!pos->ops->match(system, event,
-				argc, (const char **)argv, pos))
+				argc - 1, (const char **)argv + 1, pos))
 			continue;
 
 		ret = pos->ops->free(pos);
@@ -71,21 +79,22 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
 			break;
 	}
 	mutex_unlock(&event_mutex);
-
+out:
+	argv_free(argv);
 	return ret;
 }
 
-static int create_dyn_event(int argc, char **argv)
+static int create_dyn_event(const char *raw_command)
 {
 	struct dyn_event_operations *ops;
 	int ret = -ENODEV;
 
-	if (argv[0][0] == '-' || argv[0][0] == '!')
-		return dyn_event_release(argc, argv, NULL);
+	if (raw_command[0] == '-' || raw_command[0] == '!')
+		return dyn_event_release(raw_command, NULL);
 
 	mutex_lock(&dyn_event_ops_mutex);
 	list_for_each_entry(ops, &dyn_event_ops_list, list) {
-		ret = ops->create(argc, (const char **)argv);
+		ret = ops->create(raw_command);
 		if (!ret || ret != -ECANCELED)
 			break;
 	}
diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h
index d6857a254ede..4f4e03df4cbb 100644
--- a/kernel/trace/trace_dynevent.h
+++ b/kernel/trace/trace_dynevent.h
@@ -39,7 +39,7 @@ struct dyn_event;
  */
 struct dyn_event_operations {
 	struct list_head	list;
-	int (*create)(int argc, const char *argv[]);
+	int (*create)(const char *raw_command);
 	int (*show)(struct seq_file *m, struct dyn_event *ev);
 	bool (*is_busy)(struct dyn_event *ev);
 	int (*free)(struct dyn_event *ev);
@@ -97,7 +97,7 @@ void *dyn_event_seq_start(struct seq_file *m, loff_t *pos);
 void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos);
 void dyn_event_seq_stop(struct seq_file *m, void *v);
 int dyn_events_release_all(struct dyn_event_operations *type);
-int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type);
+int dyn_event_release(const char *raw_command, struct dyn_event_operations *type);
 
 /*
  * for_each_dyn_event	-	iterate over the dyn_event list
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d04f380f6286..446273d7813d 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -34,7 +34,7 @@ static int __init set_kprobe_boot_events(char *str)
 }
 __setup("kprobe_event=", set_kprobe_boot_events);
 
-static int trace_kprobe_create(int argc, const char **argv);
+static int trace_kprobe_create(const char *raw_command);
 static int trace_kprobe_show(struct seq_file *m, struct dyn_event *ev);
 static int trace_kprobe_release(struct dyn_event *ev);
 static bool trace_kprobe_is_busy(struct dyn_event *ev);
@@ -710,7 +710,7 @@ static inline void sanitize_event_name(char *name)
 			*name = '_';
 }
 
-static int trace_kprobe_create(int argc, const char *argv[])
+static int __trace_kprobe_create(int argc, const char *argv[])
 {
 	/*
 	 * Argument syntax:
@@ -909,20 +909,25 @@ static int trace_kprobe_create(int argc, const char *argv[])
 	goto out;
 }
 
-static int create_or_delete_trace_kprobe(int argc, char **argv)
+static int trace_kprobe_create(const char *raw_command)
+{
+	return trace_probe_create(raw_command, __trace_kprobe_create);
+}
+
+static int create_or_delete_trace_kprobe(const char *raw_command)
 {
 	int ret;
 
-	if (argv[0][0] == '-')
-		return dyn_event_release(argc, argv, &trace_kprobe_ops);
+	if (raw_command[0] == '-')
+		return dyn_event_release(raw_command, &trace_kprobe_ops);
 
-	ret = trace_kprobe_create(argc, (const char **)argv);
+	ret = trace_kprobe_create(raw_command);
 	return ret == -ECANCELED ? -EINVAL : ret;
 }
 
 static int trace_kprobe_run_command(struct dynevent_cmd *cmd)
 {
-	return trace_run_command(cmd->seq.buffer, create_or_delete_trace_kprobe);
+	return create_or_delete_trace_kprobe(cmd->seq.buffer);
 }
 
 /**
@@ -1083,7 +1088,7 @@ int kprobe_event_delete(const char *name)
 
 	snprintf(buf, MAX_EVENT_NAME_LEN, "-:%s", name);
 
-	return trace_run_command(buf, create_or_delete_trace_kprobe);
+	return create_or_delete_trace_kprobe(buf);
 }
 EXPORT_SYMBOL_GPL(kprobe_event_delete);
 
@@ -1886,7 +1891,7 @@ static __init void setup_boot_kprobe_events(void)
 		if (p)
 			*p++ = '\0';
 
-		ret = trace_run_command(cmd, create_or_delete_trace_kprobe);
+		ret = create_or_delete_trace_kprobe(cmd);
 		if (ret)
 			pr_warn("Failed to add event(%d): %s\n", ret, cmd);
 		else
@@ -1984,8 +1989,7 @@ static __init int kprobe_trace_self_tests_init(void)
 
 	pr_info("Testing kprobe tracing: ");
 
-	ret = trace_run_command("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)",
-				create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on probing function entry.\n");
 		warn++;
@@ -2006,8 +2010,7 @@ static __init int kprobe_trace_self_tests_init(void)
 		}
 	}
 
-	ret = trace_run_command("r:testprobe2 kprobe_trace_selftest_target $retval",
-				create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("r:testprobe2 kprobe_trace_selftest_target $retval");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on probing function return.\n");
 		warn++;
@@ -2080,13 +2083,13 @@ static __init int kprobe_trace_self_tests_init(void)
 				trace_probe_event_call(&tk->tp), file);
 	}
 
-	ret = trace_run_command("-:testprobe", create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("-:testprobe");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on deleting a probe.\n");
 		warn++;
 	}
 
-	ret = trace_run_command("-:testprobe2", create_or_delete_trace_kprobe);
+	ret = create_or_delete_trace_kprobe("-:testprobe2");
 	if (WARN_ON_ONCE(ret)) {
 		pr_warn("error on deleting a probe.\n");
 		warn++;
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index d2867ccc6aca..abc66d1cbf82 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -1134,3 +1134,20 @@ bool trace_probe_match_command_args(struct trace_probe *tp,
 	}
 	return true;
 }
+
+int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **))
+{
+	int argc, ret;
+	char **argv;
+
+	argv = argv_split(GFP_KERNEL, raw_command, &argc);
+	if (!argv)
+		return -ENOMEM;
+
+	if (argc)
+		ret = createfn(argc, (const char **)argv);
+
+	argv_free(argv);
+
+	return ret;
+}
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 2f703a20c724..7ce4027089ee 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -341,6 +341,7 @@ struct event_file_link *trace_probe_get_file_link(struct trace_probe *tp,
 int trace_probe_compare_arg_type(struct trace_probe *a, struct trace_probe *b);
 bool trace_probe_match_command_args(struct trace_probe *tp,
 				    int argc, const char **argv);
+int trace_probe_create(const char *raw_command, int (*createfn)(int, const char **));
 
 #define trace_probe_for_each_link(pos, tp)	\
 	list_for_each_entry(pos, &(tp)->event->files, list)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 3cf7128e1ad3..e6b56a65f80f 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -34,7 +34,7 @@ struct uprobe_trace_entry_head {
 #define DATAOF_TRACE_ENTRY(entry, is_return)		\
 	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
 
-static int trace_uprobe_create(int argc, const char **argv);
+static int trace_uprobe_create(const char *raw_command);
 static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
 static int trace_uprobe_release(struct dyn_event *ev);
 static bool trace_uprobe_is_busy(struct dyn_event *ev);
@@ -530,7 +530,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
  * Argument syntax:
  *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
  */
-static int trace_uprobe_create(int argc, const char **argv)
+static int __trace_uprobe_create(int argc, const char **argv)
 {
 	struct trace_uprobe *tu;
 	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
@@ -716,14 +716,19 @@ static int trace_uprobe_create(int argc, const char **argv)
 	return ret;
 }
 
-static int create_or_delete_trace_uprobe(int argc, char **argv)
+int trace_uprobe_create(const char *raw_command)
+{
+	return trace_probe_create(raw_command, __trace_uprobe_create);
+}
+
+static int create_or_delete_trace_uprobe(const char *raw_command)
 {
 	int ret;
 
-	if (argv[0][0] == '-')
-		return dyn_event_release(argc, argv, &trace_uprobe_ops);
+	if (raw_command[0] == '-')
+		return dyn_event_release(raw_command, &trace_uprobe_ops);
 
-	ret = trace_uprobe_create(argc, (const char **)argv);
+	ret = trace_uprobe_create(raw_command);
 	return ret == -ECANCELED ? -EINVAL : ret;
 }
 
-- 
2.25.1



-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 1/4] tracing: Make trace_*_run_command() more flexible

[Tom Zanussi]

Hi Masami,

On Mon, 2020-10-19 at 00:15 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Sun, 18 Oct 2020 23:20:11 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi Tom,
> > 
> > On Fri, 16 Oct 2020 16:48:22 -0500
> > Tom Zanussi <zanussi@kernel.org> wrote:
> > 
> > > trace_run_command() and therefore functions that use it, such as
> > > trace_parse_run_command(), uses argv_split() to split the command
> > > into
> > > an array of args then passed to the callback to handle.
> > > 
> > > This works fine for most commands but some commands would like to
> > > allow the user to use and additional separator to visually group
> > > items
> > > in the command.  One example would be synthetic event commands,
> > > which
> > > use semicolons to group fields:
> > > 
> > >   echo 'event_name int a; char b[]; u64 lat' >> synthetic_events
> > > 
> > > What gets passed as an args array to the command for a command
> > > like
> > > this include tokens that have semicolons included with the token,
> > > which the callback then needs to strip out, since argv_split()
> > > only
> > > looks at whitespace as a separator.
> > > 
> > > It would be nicer to just have trace_run_command() strip out the
> > > semicolons at its level rather than passing that task onto the
> > > callback. To accomplish that, this change adds an
> > > 'additional_sep'
> > > arg to a new __trace_run_command() function that allows a caller
> > > to
> > > pass an additional separator char that if non-zero simply
> > > replaces
> > > that character with whitespace before splitting the command into
> > > args.
> > > The original trace_run_command() remains as-is in this regard,
> > > simply
> > > calling __trace_run_command() with 0 for the separator, while
> > > making a
> > > new trace_run_command_add_sep() version that can be used to pass
> > > in a
> > > separator.
> > 
> > No, I don't like to tweak trace_run_command() that way, I'm OK to
> > delegate the argv_split() totally to the callback function (pass
> > the raw command string to the callback), OR __create_synth_event()
> > concatinate the fields argv and parse it by itself (I think the
> > latter is better and simpler).
> > 
> > The ";" separator is not an additinal separator but that is higher
> > level separator for synthetic event. Suppose that you get the
> > following input;
> >  "myevent int c ; char b"
> >  "myevent int;c;char;b;"
> > These should not be same for the synthetic events. The fields must
> > be
> > splitted by ';' at first, and then each field should be parsed.
> > 
> > So, I recommend you not to pass the additional separator to the
> > generic function, but (since it is only for synthetic event) to
> > reconstruct the raw command from argv, and parse it again with
> > ";" inside synthetic event parser. Then each fields parser can
> > check that is a wrong input or not.
> > 
> > It will be something like
> > 
> > __create_synth_event(argc, argv)
> > {
> > 	<event-name parsing>
> > 	argc--; argv++;
> > 
> > 	raw_fields = concat_argv(argc, argv);// you can assume argv is
> > generated by argv_split().
> > 	vfields = split_fields(raw_fields, &nfields);// similar to
> > argv_split()
> > 	for (i = 0; i < nfields; i++)
> > 		parse_synth_field(vfields[i]);
> > }
> > 
> > Then, you don't need to change the generic functions, and also
> > you will get the correct parser routines.
> 
> If you think the split->concat->split process is redundant, I think
> we
> can remove trace_run_command() and just pass a raw command string to
> each
> event command parser as I said above.
> 
> In this way, each event create function can parse the command by
> themselves.
> So you can parse the command as you like.
> 
> Here is the patch which modifies trace-{k,u}probe and trace-dynevent
> side, but not changing synthetic event side yet. Will this help you?
> 

Yeah, it was probably a mistake to try to shoehorn synthetic events
into the unmodified trace_run_command() in the first place.

This should work for me - I'll build on it.  Thanks for supplying it!

Tom


> From 5aed03a8df0926d92f6585b3932fdc96324cb82d Mon Sep 17 00:00:00
> 2001
> From: Masami Hiramatsu <mhiramat@kernel.org>
> Date: Sat, 17 Oct 2020 23:11:29 +0900
> Subject: [PATCH] tracing/dynevent: Delegate parsing to create
> function
> 
> Delegate command parsing to each create function so that the
> command syntax can be customized.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/trace/trace.c          | 23 ++---------------------
>  kernel/trace/trace.h          |  3 +--
>  kernel/trace/trace_dynevent.c | 35 ++++++++++++++++++++++-----------
> --
>  kernel/trace/trace_dynevent.h |  4 ++--
>  kernel/trace/trace_kprobe.c   | 33 ++++++++++++++++++---------------
>  kernel/trace/trace_probe.c    | 17 +++++++++++++++++
>  kernel/trace/trace_probe.h    |  1 +
>  kernel/trace/trace_uprobe.c   | 17 +++++++++++------
>  8 files changed, 74 insertions(+), 59 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 63c97012ed39..277d97220971 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -9367,30 +9367,11 @@ void ftrace_dump(enum ftrace_dump_mode
> oops_dump_mode)
>  }
>  EXPORT_SYMBOL_GPL(ftrace_dump);
>  
> -int trace_run_command(const char *buf, int (*createfn)(int, char
> **))
> -{
> -	char **argv;
> -	int argc, ret;
> -
> -	argc = 0;
> -	ret = 0;
> -	argv = argv_split(GFP_KERNEL, buf, &argc);
> -	if (!argv)
> -		return -ENOMEM;
> -
> -	if (argc)
> -		ret = createfn(argc, argv);
> -
> -	argv_free(argv);
> -
> -	return ret;
> -}
> -
>  #define WRITE_BUFSIZE  4096
>  
>  ssize_t trace_parse_run_command(struct file *file, const char __user
> *buffer,
>  				size_t count, loff_t *ppos,
> -				int (*createfn)(int, char **))
> +				int (*createfn)(const char *))
>  {
>  	char *kbuf, *buf, *tmp;
>  	int ret = 0;
> @@ -9438,7 +9419,7 @@ ssize_t trace_parse_run_command(struct file
> *file, const char __user *buffer,
>  			if (tmp)
>  				*tmp = '\0';
>  
> -			ret = trace_run_command(buf, createfn);
> +			ret = createfn(buf);
>  			if (ret)
>  				goto out;
>  			buf += size;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 34e0c4d5a6e7..02d7c487a30b 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1982,10 +1982,9 @@ extern int tracing_set_cpumask(struct
> trace_array *tr,
>  
>  #define MAX_EVENT_NAME_LEN	64
>  
> -extern int trace_run_command(const char *buf, int (*createfn)(int,
> char**));
>  extern ssize_t trace_parse_run_command(struct file *file,
>  		const char __user *buffer, size_t count, loff_t *ppos,
> -		int (*createfn)(int, char**));
> +		int (*createfn)(const char *));
>  
>  extern unsigned int err_pos(char *cmd, const char *str);
>  extern void tracing_log_err(struct trace_array *tr,
> diff --git a/kernel/trace/trace_dynevent.c
> b/kernel/trace/trace_dynevent.c
> index 5fa49cfd2bb6..af83bc5447fe 100644
> --- a/kernel/trace/trace_dynevent.c
> +++ b/kernel/trace/trace_dynevent.c
> @@ -31,23 +31,31 @@ int dyn_event_register(struct
> dyn_event_operations *ops)
>  	return 0;
>  }
>  
> -int dyn_event_release(int argc, char **argv, struct
> dyn_event_operations *type)
> +int dyn_event_release(const char *raw_command, struct
> dyn_event_operations *type)
>  {
>  	struct dyn_event *pos, *n;
>  	char *system = NULL, *event, *p;
> -	int ret = -ENOENT;
> +	int argc, ret = -ENOENT;
> +	char **argv;
> +
> +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> +	if (!argv)
> +		return -ENOMEM;
>  
>  	if (argv[0][0] == '-') {
> -		if (argv[0][1] != ':')
> -			return -EINVAL;
> +		if (argv[0][1] != ':') {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  		event = &argv[0][2];
>  	} else {
>  		event = strchr(argv[0], ':');
> -		if (!event)
> -			return -EINVAL;
> +		if (!event) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  		event++;
>  	}
> -	argc--; argv++;
>  
>  	p = strchr(event, '/');
>  	if (p) {
> @@ -63,7 +71,7 @@ int dyn_event_release(int argc, char **argv, struct
> dyn_event_operations *type)
>  		if (type && type != pos->ops)
>  			continue;
>  		if (!pos->ops->match(system, event,
> -				argc, (const char **)argv, pos))
> +				argc - 1, (const char **)argv + 1,
> pos))
>  			continue;
>  
>  		ret = pos->ops->free(pos);
> @@ -71,21 +79,22 @@ int dyn_event_release(int argc, char **argv,
> struct dyn_event_operations *type)
>  			break;
>  	}
>  	mutex_unlock(&event_mutex);
> -
> +out:
> +	argv_free(argv);
>  	return ret;
>  }
>  
> -static int create_dyn_event(int argc, char **argv)
> +static int create_dyn_event(const char *raw_command)
>  {
>  	struct dyn_event_operations *ops;
>  	int ret = -ENODEV;
>  
> -	if (argv[0][0] == '-' || argv[0][0] == '!')
> -		return dyn_event_release(argc, argv, NULL);
> +	if (raw_command[0] == '-' || raw_command[0] == '!')
> +		return dyn_event_release(raw_command, NULL);
>  
>  	mutex_lock(&dyn_event_ops_mutex);
>  	list_for_each_entry(ops, &dyn_event_ops_list, list) {
> -		ret = ops->create(argc, (const char **)argv);
> +		ret = ops->create(raw_command);
>  		if (!ret || ret != -ECANCELED)
>  			break;
>  	}
> diff --git a/kernel/trace/trace_dynevent.h
> b/kernel/trace/trace_dynevent.h
> index d6857a254ede..4f4e03df4cbb 100644
> --- a/kernel/trace/trace_dynevent.h
> +++ b/kernel/trace/trace_dynevent.h
> @@ -39,7 +39,7 @@ struct dyn_event;
>   */
>  struct dyn_event_operations {
>  	struct list_head	list;
> -	int (*create)(int argc, const char *argv[]);
> +	int (*create)(const char *raw_command);
>  	int (*show)(struct seq_file *m, struct dyn_event *ev);
>  	bool (*is_busy)(struct dyn_event *ev);
>  	int (*free)(struct dyn_event *ev);
> @@ -97,7 +97,7 @@ void *dyn_event_seq_start(struct seq_file *m,
> loff_t *pos);
>  void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos);
>  void dyn_event_seq_stop(struct seq_file *m, void *v);
>  int dyn_events_release_all(struct dyn_event_operations *type);
> -int dyn_event_release(int argc, char **argv, struct
> dyn_event_operations *type);
> +int dyn_event_release(const char *raw_command, struct
> dyn_event_operations *type);
>  
>  /*
>   * for_each_dyn_event	-	iterate over the dyn_event list
> diff --git a/kernel/trace/trace_kprobe.c
> b/kernel/trace/trace_kprobe.c
> index d04f380f6286..446273d7813d 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -34,7 +34,7 @@ static int __init set_kprobe_boot_events(char *str)
>  }
>  __setup("kprobe_event=", set_kprobe_boot_events);
>  
> -static int trace_kprobe_create(int argc, const char **argv);
> +static int trace_kprobe_create(const char *raw_command);
>  static int trace_kprobe_show(struct seq_file *m, struct dyn_event
> *ev);
>  static int trace_kprobe_release(struct dyn_event *ev);
>  static bool trace_kprobe_is_busy(struct dyn_event *ev);
> @@ -710,7 +710,7 @@ static inline void sanitize_event_name(char
> *name)
>  			*name = '_';
>  }
>  
> -static int trace_kprobe_create(int argc, const char *argv[])
> +static int __trace_kprobe_create(int argc, const char *argv[])
>  {
>  	/*
>  	 * Argument syntax:
> @@ -909,20 +909,25 @@ static int trace_kprobe_create(int argc, const
> char *argv[])
>  	goto out;
>  }
>  
> -static int create_or_delete_trace_kprobe(int argc, char **argv)
> +static int trace_kprobe_create(const char *raw_command)
> +{
> +	return trace_probe_create(raw_command, __trace_kprobe_create);
> +}
> +
> +static int create_or_delete_trace_kprobe(const char *raw_command)
>  {
>  	int ret;
>  
> -	if (argv[0][0] == '-')
> -		return dyn_event_release(argc, argv,
> &trace_kprobe_ops);
> +	if (raw_command[0] == '-')
> +		return dyn_event_release(raw_command,
> &trace_kprobe_ops);
>  
> -	ret = trace_kprobe_create(argc, (const char **)argv);
> +	ret = trace_kprobe_create(raw_command);
>  	return ret == -ECANCELED ? -EINVAL : ret;
>  }
>  
>  static int trace_kprobe_run_command(struct dynevent_cmd *cmd)
>  {
> -	return trace_run_command(cmd->seq.buffer,
> create_or_delete_trace_kprobe);
> +	return create_or_delete_trace_kprobe(cmd->seq.buffer);
>  }
>  
>  /**
> @@ -1083,7 +1088,7 @@ int kprobe_event_delete(const char *name)
>  
>  	snprintf(buf, MAX_EVENT_NAME_LEN, "-:%s", name);
>  
> -	return trace_run_command(buf, create_or_delete_trace_kprobe);
> +	return create_or_delete_trace_kprobe(buf);
>  }
>  EXPORT_SYMBOL_GPL(kprobe_event_delete);
>  
> @@ -1886,7 +1891,7 @@ static __init void
> setup_boot_kprobe_events(void)
>  		if (p)
>  			*p++ = '\0';
>  
> -		ret = trace_run_command(cmd,
> create_or_delete_trace_kprobe);
> +		ret = create_or_delete_trace_kprobe(cmd);
>  		if (ret)
>  			pr_warn("Failed to add event(%d): %s\n", ret,
> cmd);
>  		else
> @@ -1984,8 +1989,7 @@ static __init int
> kprobe_trace_self_tests_init(void)
>  
>  	pr_info("Testing kprobe tracing: ");
>  
> -	ret = trace_run_command("p:testprobe
> kprobe_trace_selftest_target $stack $stack0 +0($stack)",
> -				create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("p:testprobe
> kprobe_trace_selftest_target $stack $stack0 +0($stack)");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on probing function entry.\n");
>  		warn++;
> @@ -2006,8 +2010,7 @@ static __init int
> kprobe_trace_self_tests_init(void)
>  		}
>  	}
>  
> -	ret = trace_run_command("r:testprobe2
> kprobe_trace_selftest_target $retval",
> -				create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("r:testprobe2
> kprobe_trace_selftest_target $retval");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on probing function return.\n");
>  		warn++;
> @@ -2080,13 +2083,13 @@ static __init int
> kprobe_trace_self_tests_init(void)
>  				trace_probe_event_call(&tk->tp), file);
>  	}
>  
> -	ret = trace_run_command("-:testprobe",
> create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("-:testprobe");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on deleting a probe.\n");
>  		warn++;
>  	}
>  
> -	ret = trace_run_command("-:testprobe2",
> create_or_delete_trace_kprobe);
> +	ret = create_or_delete_trace_kprobe("-:testprobe2");
>  	if (WARN_ON_ONCE(ret)) {
>  		pr_warn("error on deleting a probe.\n");
>  		warn++;
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index d2867ccc6aca..abc66d1cbf82 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -1134,3 +1134,20 @@ bool trace_probe_match_command_args(struct
> trace_probe *tp,
>  	}
>  	return true;
>  }
> +
> +int trace_probe_create(const char *raw_command, int (*createfn)(int,
> const char **))
> +{
> +	int argc, ret;
> +	char **argv;
> +
> +	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> +	if (!argv)
> +		return -ENOMEM;
> +
> +	if (argc)
> +		ret = createfn(argc, (const char **)argv);
> +
> +	argv_free(argv);
> +
> +	return ret;
> +}
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 2f703a20c724..7ce4027089ee 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -341,6 +341,7 @@ struct event_file_link
> *trace_probe_get_file_link(struct trace_probe *tp,
>  int trace_probe_compare_arg_type(struct trace_probe *a, struct
> trace_probe *b);
>  bool trace_probe_match_command_args(struct trace_probe *tp,
>  				    int argc, const char **argv);
> +int trace_probe_create(const char *raw_command, int (*createfn)(int,
> const char **));
>  
>  #define trace_probe_for_each_link(pos, tp)	\
>  	list_for_each_entry(pos, &(tp)->event->files, list)
> diff --git a/kernel/trace/trace_uprobe.c
> b/kernel/trace/trace_uprobe.c
> index 3cf7128e1ad3..e6b56a65f80f 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -34,7 +34,7 @@ struct uprobe_trace_entry_head {
>  #define DATAOF_TRACE_ENTRY(entry, is_return)		\
>  	((void*)(entry) + SIZEOF_TRACE_ENTRY(is_return))
>  
> -static int trace_uprobe_create(int argc, const char **argv);
> +static int trace_uprobe_create(const char *raw_command);
>  static int trace_uprobe_show(struct seq_file *m, struct dyn_event
> *ev);
>  static int trace_uprobe_release(struct dyn_event *ev);
>  static bool trace_uprobe_is_busy(struct dyn_event *ev);
> @@ -530,7 +530,7 @@ static int register_trace_uprobe(struct
> trace_uprobe *tu)
>   * Argument syntax:
>   *  - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)]
> [FETCHARGS]
>   */
> -static int trace_uprobe_create(int argc, const char **argv)
> +static int __trace_uprobe_create(int argc, const char **argv)
>  {
>  	struct trace_uprobe *tu;
>  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
> @@ -716,14 +716,19 @@ static int trace_uprobe_create(int argc, const
> char **argv)
>  	return ret;
>  }
>  
> -static int create_or_delete_trace_uprobe(int argc, char **argv)
> +int trace_uprobe_create(const char *raw_command)
> +{
> +	return trace_probe_create(raw_command, __trace_uprobe_create);
> +}
> +
> +static int create_or_delete_trace_uprobe(const char *raw_command)
>  {
>  	int ret;
>  
> -	if (argv[0][0] == '-')
> -		return dyn_event_release(argc, argv,
> &trace_uprobe_ops);
> +	if (raw_command[0] == '-')
> +		return dyn_event_release(raw_command,
> &trace_uprobe_ops);
>  
> -	ret = trace_uprobe_create(argc, (const char **)argv);
> +	ret = trace_uprobe_create(raw_command);
>  	return ret == -ECANCELED ? -EINVAL : ret;
>  }
>  
> -- 
> 2.25.1
> 
> 
> 

Re: [PATCH 1/4] tracing: Make trace_*_run_command() more flexible

[Masami Hiramatsu]

Hi Tom,

On Mon, 19 Oct 2020 09:35:32 -0500
Tom Zanussi <zanussi@kernel.org> wrote:

> Hi Masami,
> 
> On Mon, 2020-10-19 at 00:15 +0900, Masami Hiramatsu wrote:
> > Hi Tom,
> > 
> > On Sun, 18 Oct 2020 23:20:11 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Hi Tom,
> > > 
> > > On Fri, 16 Oct 2020 16:48:22 -0500
> > > Tom Zanussi <zanussi@kernel.org> wrote:
> > > 
> > > > trace_run_command() and therefore functions that use it, such as
> > > > trace_parse_run_command(), uses argv_split() to split the command
> > > > into
> > > > an array of args then passed to the callback to handle.
> > > > 
> > > > This works fine for most commands but some commands would like to
> > > > allow the user to use and additional separator to visually group
> > > > items
> > > > in the command.  One example would be synthetic event commands,
> > > > which
> > > > use semicolons to group fields:
> > > > 
> > > >   echo 'event_name int a; char b[]; u64 lat' >> synthetic_events
> > > > 
> > > > What gets passed as an args array to the command for a command
> > > > like
> > > > this include tokens that have semicolons included with the token,
> > > > which the callback then needs to strip out, since argv_split()
> > > > only
> > > > looks at whitespace as a separator.
> > > > 
> > > > It would be nicer to just have trace_run_command() strip out the
> > > > semicolons at its level rather than passing that task onto the
> > > > callback. To accomplish that, this change adds an
> > > > 'additional_sep'
> > > > arg to a new __trace_run_command() function that allows a caller
> > > > to
> > > > pass an additional separator char that if non-zero simply
> > > > replaces
> > > > that character with whitespace before splitting the command into
> > > > args.
> > > > The original trace_run_command() remains as-is in this regard,
> > > > simply
> > > > calling __trace_run_command() with 0 for the separator, while
> > > > making a
> > > > new trace_run_command_add_sep() version that can be used to pass
> > > > in a
> > > > separator.
> > > 
> > > No, I don't like to tweak trace_run_command() that way, I'm OK to
> > > delegate the argv_split() totally to the callback function (pass
> > > the raw command string to the callback), OR __create_synth_event()
> > > concatinate the fields argv and parse it by itself (I think the
> > > latter is better and simpler).
> > > 
> > > The ";" separator is not an additinal separator but that is higher
> > > level separator for synthetic event. Suppose that you get the
> > > following input;
> > >  "myevent int c ; char b"
> > >  "myevent int;c;char;b;"
> > > These should not be same for the synthetic events. The fields must
> > > be
> > > splitted by ';' at first, and then each field should be parsed.
> > > 
> > > So, I recommend you not to pass the additional separator to the
> > > generic function, but (since it is only for synthetic event) to
> > > reconstruct the raw command from argv, and parse it again with
> > > ";" inside synthetic event parser. Then each fields parser can
> > > check that is a wrong input or not.
> > > 
> > > It will be something like
> > > 
> > > __create_synth_event(argc, argv)
> > > {
> > > 	<event-name parsing>
> > > 	argc--; argv++;
> > > 
> > > 	raw_fields = concat_argv(argc, argv);// you can assume argv is
> > > generated by argv_split().
> > > 	vfields = split_fields(raw_fields, &nfields);// similar to
> > > argv_split()
> > > 	for (i = 0; i < nfields; i++)
> > > 		parse_synth_field(vfields[i]);
> > > }
> > > 
> > > Then, you don't need to change the generic functions, and also
> > > you will get the correct parser routines.
> > 
> > If you think the split->concat->split process is redundant, I think
> > we
> > can remove trace_run_command() and just pass a raw command string to
> > each
> > event command parser as I said above.
> > 
> > In this way, each event create function can parse the command by
> > themselves.
> > So you can parse the command as you like.
> > 
> > Here is the patch which modifies trace-{k,u}probe and trace-dynevent
> > side, but not changing synthetic event side yet. Will this help you?
> > 
> 
> Yeah, it was probably a mistake to try to shoehorn synthetic events
> into the unmodified trace_run_command() in the first place.
> 
> This should work for me - I'll build on it.  Thanks for supplying it!

Yeah, feel free to merge your patch if you need, since this is
incomplete patch, not good for bisecting.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>