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

> 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.

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?