Messages in this thread | | | Date | Tue, 20 Oct 2020 22:20:24 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH 1/4] tracing: Make trace_*_run_command() more flexible |
| |
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>
| |