Re: [PATCH v3 25/33] tracing: Allow whitespace to surround hist trigger filter

[Tom Zanussi]

Hi Steve,

On Wed, 2017-10-04 at 14:11 -0400, Steven Rostedt wrote:
> On Fri, 22 Sep 2017 15:00:05 -0500
> Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx> wrote:
> 
> > The existing code only allows for one space before and after the 'if'
> > specifying the filter for a hist trigger.  Add code to make that more
> > permissive as far as whitespace goes.  Specifically, we want to allow
> > spaces in the trigger itself now that we have additional syntax
> > (onmatch/onmax) where spaces are more natural e.g. spaces after commas
> > in param lists.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx>
> > ---
> >  kernel/trace/trace_events_hist.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index ba42fe2..431f2b2 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -4857,7 +4857,7 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
> >  	struct synth_event *se;
> >  	const char *se_name;
> >  	bool remove = false;
> > -	char *trigger;
> > +	char *trigger, *p;
> >  	int ret = 0;
> >  
> >  	if (!param)
> > @@ -4866,10 +4866,23 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
> >  	if (glob[0] == '!')
> >  		remove = true;
> >  
> > -	/* separate the trigger from the filter (k:v [if filter]) */
> > -	trigger = strsep(&param, " \t");
> > -	if (!trigger)
> > -		return -EINVAL;
> > +	/*
> > +	 * separate the trigger from the filter (k:v [if filter])
> > +	 * allowing for whitespace in the trigger
> > +	 */
> > +	trigger = param;
> > +	p = strstr(param, " if");
> > +	if (!p)
> > +		p = strstr(param, "\tif");
> > +	if (p) {
> > +		if (p == trigger)
> > +			return -EINVAL;
> > +		param = p + 1;
> > +		param = strstrip(param);
> > +		*p = '\0';
> > +		trigger = strstrip(trigger);
> > +	} else
> > +		param = NULL;
> 
> I think you forgot to update this:
> 

I was going to but on closer inspection realized the simpler form
wouldn't accomplish the same thing - the problem this is trying to solve
is to allow bits of whitespace within the trigger (because we now have
function-like syntax, which should allow whitespace after commas for
instance) and separating the trigger from the filter ('if').  So we
explicitly search for 'if' with preceding whitespace, which strsep won't
accomplish.

So while it may not be pretty, it accomplishes that, while the below
won't (it would create a truncated trigger if there were e.g. commas
followed by spaces).

Tom

> > > This seems rather complex. Wouldn't the following work?
> > > 
> > > 	param = skip_spaces(param);
> > > 	trigger = strsep(&param, " \t");
> > > 	if (param)
> > > 		param = strstrip(param);
> > >   
> > 
> > Yes, much better ;-)
> > 
> > Tom
> 
> -- Steve
> 
> >  
> >  	attrs = parse_hist_trigger_attrs(trigger);
> >  	if (IS_ERR(attrs))
> > @@ -4927,6 +4940,7 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
> >  	}
> >  
> >  	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> > +
> >  	/*
> >  	 * The above returns on success the # of triggers registered,
> >  	 * but if it didn't register any it returns zero.  Consider no
>