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