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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>