Re: [PATCH] tracing: Don't display trigger file for events that don't have register func

[Chunyu Hu]

----- Original Message -----
> From: "Steven Rostedt" <rostedt@xxxxxxxxxxx>
> To: "Chunyu Hu" <chuhu@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Sent: Tuesday, May 3, 2016 10:42:44 AM
> Subject: Re: [PATCH] tracing: Don't display trigger file for events that don't have register func
> 
> On Mon,  2 May 2016 22:14:14 +0800
> Chunyu Hu <chuhu@xxxxxxxxxx> wrote:
> 
> > Currently register function of the event will be called
> > through the 'reg'  field of event class directly without
> > any check when seting up triggers.
> > 
> > Triggers for events that don't support register through
> > debug fs (events under events/ftrace are for perf to
> 
> Actually, they were created for trace-cmd. I'm not even sure if perf
> uses the ftrace event formats.

Thanks for correcting me. I misunderstood it.
 
> 
> > read event format, and most of them don't have regisgter
> > function except events/ftrace/function.) can't be enabled
> > at all, and an oops will be hit when setting up trigger
> > for those events, so just not showing them is an easy way
> > to avoid the oops.
> > 
> > Signed-off-by: Chunyu Hu <chuhu@xxxxxxxxxx>
> > ---
> >  kernel/trace/trace_events.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index da1eeb6..9fb99fd 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -2138,9 +2138,10 @@ event_create_dir(struct dentry *parent, struct
> > trace_event_file *file)
> >  	trace_create_file("filter", 0644, file->dir, file,
> >  			  &ftrace_event_filter_fops);
> >  
> > -	trace_create_file("trigger", 0644, file->dir, file,
> > -			  &event_trigger_fops);
> > -
> > +	if (call->class->reg) {
> > +		trace_create_file("trigger", 0644, file->dir, file,
> > +				  &event_trigger_fops);
> > +	}
> 
> As you stated, reg is there for function tracing, and is not a good
> value to use as a check. Use the following check instead:
> 
> 	if (!(call->flags & TRACE_EVENT_FL_IGNORE_ENABLE))
> 		trace_create_file("trigger", 0644, file->dir, file,
> 				  &event_trigger_fops);

Thanks. Agree, will prepare v2 for this.

It looks like that checking both is safest way, like other parts
did in trace_events.c. but i didn't see other events using 
TRACE_EVENT_FL_IGNORE_ENABLE, ftrace event sub system is the only user now.
and didn't see other events that don't have register func.
so i think it's safe to use only this flag to check it.

> And add a comment that states that only event directories that can be
> enabled should have triggers.

Ok.

> -- Steve
> 
> 
> 
> >  #ifdef CONFIG_HIST_TRIGGERS
> >  	trace_create_file("hist", 0444, file->dir, file,
> >  			  &event_hist_fops);
> 
> 

-- 
Regards,
Chunyu Hu