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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1333715093.23090.2.camel@pippen.local.home>
Date:	Fri, 06 Apr 2012 08:24:53 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mark Asselstine <mark.asselstine@...driver.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] trace-cmd: setting plugin to 'nop' clears data
 before its recorded

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/