Re: [PATCH v3 3/5] tracing: Update synth command errors

[Tom Zanussi <zanussi@kernel.org>]

Hi Steve,

On Mon, 2020-12-07 at 20:13 -0500, Steven Rostedt wrote:
> On Mon, 26 Oct 2020 10:06:11 -0500
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > 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 add INVALID_CMD and INVALID_DYN_CMD
> > to
> > catch and display the correct form for badly-formed commands, which
> > can also be used in place of CMD_INCOMPLETE, which is removed, and
> > remove CMD_TOO_LONG, since it's no longer used.
> > 
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> > ---
> 
> Unfortunately, this patch series breaks user space.
> 
> I already have scripts that do the histograms, and I'm sure others
> may
> have that too, and if we change how synthetic events are created, it
> will break them.
> 
> What's the rationale for the new delimiters?
> 

The overall problem this is trying to fix is that it was probably a
mistake to try to shoehorn the synthetic event parsing into what was
available from  trace_run_command() and trace_parse_run_command(),
which use argv_split() to do the command splitting, and which only
splits on whitespace.  Whereas the synthetic events have a bit of a
higher-level structure which is 'event field; field; field;...'

So this patchset tries to remedy that - the first patch,
(tracing/dynevent: Delegate parsing to create function) is from Masami,
and makes it possible to share code between kprobe/uprobe and synthetic
evnents, and to allow synthetic events to have their own higher-level
parsing, which the next 2 patches do.

The history in more detail:

Initially the problem was to fix the errors mentioned by Masami in
[1]. 

Things like:

  # echo myevent char str[];; int v >> synthetic_events

which was identified as INVALID_TYPE where it should just be a void arg
and

  # echo mye;vent char str[] >> synthetic_events

which was identified as BAD_NAME where it should have been an invalid
command, etc.

I suggested that the way to fix them was to consider semicolon as
additional whitespace and the result was the patchset containing [2],
which also explains the reasons for wanting to enforce semicolon
grouping.

Masami pointed out that it really wasn't correct to do it that way,
and  the commands should be split out first at the higher level by
semicolon and then further processed [3].

Unfortunately, you're correct, if you have a script that creates a
synthetic event without semicolons, this patchset will break it, as I
myself found out and fixed in patch 4 ([PATCH v3 4/5] selftests/ftrace:
Add synthetic event field separators) [4].

So whereas before this would work, even though it shouldn't have in the
first place:

  # echo 'wakeup_latency  u64 lat pid_t pid char comm[16]' >
synthetic_events

it now has to be:

  # echo 'wakeup_latency  u64 lat; pid_t pid; char comm[16]' >
synthetic_events

So yeah, this patchset fixes a set of parsing bugs for things that
shouldn't have been accepted as valid, but shouldn't break things that
are obviously valid.

If it's too late to fix them, though, I guess we'll just have to live
with them, or some other option?

Tom

[1] https://lore.kernel.org/lkml/20201014110636.139df7be275d40a23b523b84@kernel.org/
[2] https://lore.kernel.org/lkml/e29c3ae1fc46892ec792d6f6f910f75d0e12584c.1602883818.git.zanussi@kernel.org/
[3] https://lore.kernel.org/lkml/20201018232011.38e5da51f5cd8e73e6f529ee@kernel.org/
[4] https://lore.kernel.org/lkml/75a2816b4001e04e7d60bcc87aa91477ad5d90b3.1603723933.git.zanussi@kernel.org/



> -- Steve