Re: [PATCH 3/3] trace-cmd: setting plugin to 'nop' clears data before its recorded

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 2012-04-06 at 08:06 -0400, Mark Asselstine wrote:

> Works for me. I see you make use of gboolean elsewhere so I assume you prefer 
> the use of this rather than using int?

Sure.

> 
> > Also, and this has nothing to do with you or your patches, I simply hate
> > the fact that I called these plugins. They should have been called
> > "tracers" but I think I'm stuck with it. The term now is ambiguous as it
> > means both a tracer (as it does here) and it means actual plugins that
> > you can load into trace-cmd itself.
> >
> 
> To stop the proliferation do you want me to name the bool 
> 'disable_current_tracer' rather than 'disable_plugin' or 'plugin'?

disable_tracer would due.

> 
>  
> > >  {
> > >  
> > >  	disable_tracing();
> > > 
> > > -	set_plugin("nop");
> > > 
> > >  	reset_events();
> > >  	
> > >  	/* Force close and reset of ftrace pid file */
> > > 
> > > @@ -911,6 +910,12 @@ static void disable_all(void)
> > > 
> > >  	clear_trace();
> > >  
> > >  }
> > > 
> > > +static void disable_all(void)
> > > +{
> > > +	disable_all_but_plugin();
> > > +	set_plugin("nop");
> > > +}
> > > +
> > > 
> > >  static void
> > >  update_sched_event(struct event_list **event, const char *file,
> > >  
> > >  		   const char *pid_filter, const char *field_filter)
> > > 
> > > @@ -2227,7 +2232,7 @@ void trace_record (int argc, char **argv)
> > > 
> > >  	}
> > >  	
> > >  	if (!keep)
> > > 
> > > -		disable_all();
> > > +		disable_all_but_plugin();
> > > 
> > >  	printf("Kernel buffer statistics:\n"
> > >  	
> > >  	       "  Note: \"entries\" are the entries left in the
> > >  	       kernel ring buffer and are not\n"> 
> > > @@ -2245,6 +2250,8 @@ void trace_record (int argc, char **argv)
> > > 
> > >  	record_data(date2ts);
> > >  	delete_thread_data();
> > > 
> > > +	if (!keep)
> > > +		set_plugin("nop");
> > > 
> > >  	if (keep)
> > >  	
> > >  		exit(0);
> > 
> > I think the above would have been better if you did:
> > 
> > 	if (keep)
> > 		exit(0);
> > 	else
> > 		set_plugin(nop);
> > 
> > -- Steve
> 
> Wow, I wasn't being too bright there was I? Agreed and the 'else' isn't 
> necessary either, so this is a good cleanup.
> 
> I will get this fixed up later today. Do you want a V2 of the full patch series 
> or just an update of this one patch?

You can just update this patch with a v2 on it. I'm currently traveling
home from a conference and wont be back to work till Tuesday. Please,
take your time.

Thanks!

-- Steve