On Mon, 2013-08-26 at 15:34 +0900, Masami Hiramatsu wrote: > Hi Tom, > > This patch basically good for me. > > Unfortunately, I've found a small build issue on this patch. > > CC kernel/trace/trace_events_filter.o > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events.c:1864:1: error: static > declaration of 'find_event_file' follows non-static declaration > /home/mhiramat/ksrc/linux-3/kernel/trace/trace.h:1019:34: note: previous > declaration of 'find_event_file' was here > make[3]: *** [kernel/trace/trace_events.o] Error 1 >
Right, making find_event_file() extern is premature here - it isn't even used until patch 06. > > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > > index b1227b9..1733ac9 100644 > > --- a/kernel/trace/trace.h > > +++ b/kernel/trace/trace.h > > @@ -1016,9 +1016,184 @@ extern void trace_event_enable_cmd_record(bool > > enable); > > extern int event_trace_add_tracer(struct dentry *parent, struct > > trace_array *tr); > > extern int event_trace_del_tracer(struct trace_array *tr); > > > > +extern struct ftrace_event_file *find_event_file(struct trace_array *tr, > > + const char *system, > > + const char *event); > > + > > Here, you exposed find_event_file, but forgot to remove static from the > definition > in kernel/trace/trace_events.c. > And even if it solved, I also hit an error and many warnings. > > LD arch/x86/built-in.o > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c: In function > 'event_trigger_callback': > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:510:14: > error: 'struct event_trigger_data' has no member named 'post_trigger' > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c: At top level: > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:242:19: > warning: 'register_event_command' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:267:19: > warning: 'unregister_event_command' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:297:1: > warning: 'event_trigger_print' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:326:1: > warning: 'event_trigger_init' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:344:1: > warning: 'event_trigger_free' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:412:12: > warning: 'register_trigger' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:451:13: > warning: 'unregister_trigger' defined but not used [-Wunused-function] > /home/mhiramat/ksrc/linux-3/kernel/trace/trace_events_trigger.c:482:1: > warning: 'event_trigger_callback' defined but not used [-Wunused-function] > make[3]: *** [kernel/trace/trace_events_trigger.o] Error 1 > > The post_trigger member is introduced in 7/10, and all warning helper > functions > are used in the next (3/10) patch. So, I recommend you to move all > post_trigger > related code into 7/10 or merge it into this patch, and move all helper > functions > into 3/10 for fixing these warnings and an error. :) > Yeah, some of these also got left behind (or ahead) in the shuffle between patches - let me fix them up again.. > (2013/08/23 8:27), Tom Zanussi wrote: > > +/** > > + * struct event_command - callbacks and data members for event commands > > + * > [...] > > + * @post_trigger: A flag that says whether or not this command needs > [...] > > + trigger_data->count = -1; > > + trigger_data->ops = trigger_ops; > > + trigger_data->cmd_ops = cmd_ops; > > + trigger_data->mode = cmd_ops->trigger_mode; > > + trigger_data->post_trigger = cmd_ops->post_trigger; > > BTW, why the trigger_data has a copy of trigger_mode and post_trigger? > It seems redundant... :( Good point, no sooner do I assign cmd_ops than apparently completely forget about it in the next two lines. ;-) > Also, the "trigger_mode" name is not good for me, because we already has > FTRACE_EVENT_FL_TRIGGER_MODE_BIT bitflag. Perhaps, trigger_group or just > group_id may be better, I think. > > Thank you, > Right, since it's doing double duty, it was hard to come up with a good name for it - group_id might just be generic enough though - I'll switch to that in the next revision, I think.. Thanks, Tom -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
