linux-kernel - Re: [PATCH 3/3] perf inject: Handle output file via perf_data

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: <20131126130712.GK1267@krava.brq.redhat.com>
Date:	Tue, 26 Nov 2013 14:07:13 +0100
From:	Jiri Olsa <jolsa@...hat.com>
To:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Mike Galbraith <efault@....de>,
	Stephane Eranian <eranian@...gle.com>,
	David Ahern <dsahern@...il.com>,
	Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 3/3] perf inject: Handle output file via perf_data_file
 object

On Tue, Nov 26, 2013 at 09:42:13AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 26, 2013 at 11:03:24AM +0100, Jiri Olsa escreveu:
> > On Mon, Nov 25, 2013 at 04:40:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Nov 22, 2013 at 03:24:28PM +0100, Jiri Olsa escreveu:
> > > > Using the perf_data_file object to handle output
>  
> > SNIP
>  
> > > > +	if (!perf_data_file__is_pipe(&inject->output))
> > > >  		return 0;
> 
> > > >  	return perf_event__repipe_synth(tool, event);
> > > > @@ -351,10 +343,12 @@ static int __cmd_inject(struct perf_inject *inject)
> > > >  {
> > > >  	struct perf_session *session;
> > > >  	int ret = -EINVAL;
> > > > -	struct perf_data_file file = {
> > > > +	struct perf_data_file file_in = {
> > > 
> > > Why don't leave it as 'file', and on a follow up patch _then_ rename it
> > > to file_in? This way patch review gets easier, i.e. try avoiding doing
> > > multiple things per patch.
> > 
> > the input file needed to be renamed, because new 'output' file was added
> 
> Why? Is 'file' going to be reused somehow?

nope, just having file_in and file_out seemed symmetric,
but nevermind.. I can switch to file and file_out ;-)

SNIP

> > 
> > well, inject->output->fd is used on 2 places within the function,
> > so it seems logical to put it into variable and use it like that
> 
> What I'm trying to convey here is that for both this case and the other,
> having looking at these two lines:
> 
> -                 inject->output
> +                 inject->output->fd
> 
> Makes me instantaneously understand that inject->output now
> encapsulates, among other things (probably), the file descriptor that
> was called just inject->output, i.e. this patch probably isn't doing
> anything more than using a new abstraction, the code flow probably
> wasn't altered.

yes, that's what happened.. encapsulating output file processing
and getting rid of common code that's now handled by perf_data_file
object..

> 
> I.e. the smaller the patch, the better.

I dont think thats always true.. I prefer readable code
despite of 'unreadable' patches ;-)

> 
> > > > -	if (!inject->pipe_output) {
> > > > +	if (!perf_data_file__is_pipe(file_out)) {
> > > >  		session->header.data_size = inject->bytes_written;
> > > > -		perf_session__write_header(session, session->evlist, inject->output, true);
> +		perf_session__write_header(session, session->evlist, inject->output->fd, true);
> > > 
> > > Why a line for 'true' all by itself?
> > 
> > line was crossing 80 chars limit
> 
> [1]+  Stopped                 mutt
> [acme@zoo ~]$ 
> [acme@zoo ~]$ echo $COLUMNS
> 173
> [acme@zoo ~]$ 
> 
> I'm not really that strict with that old convention :-\ All in one line
> would make it ~95 columns, not a big deal, even more since it _was_
> already more than 80 columns.

I see.. I'm using checkpatch script that screams about this
so I tend to keep to that rule

> 
> I.e. your change was to replace 'inject->output' with 'out_fd', but you
> did that _and_ reflowed, i.e. two changes into one. ;-)

the smaller the patch, the better, right? :-)

> 
> Looking at this line makes me think: why do we have to pass 'session'
> _and_ 'session->evlist', i.e. the 2nd parameter can be obtained from the
> 1st. Fixing that could get us more compact code _and_ a shorter line.

we do actually, and the reason is this code, because
the session keeps the input file, while we are using
it for output file..

> 
> Will check that.
> 
> > > > -		OPT_STRING('o', "output", &output_name, "file",
> > > > +		OPT_STRING('o', "output", &inject.output.path, "file",
> > > 
> > > see, here you directly access a perf_data_file member instead of having
> > > another wrapper :-)
> > 
> > yes
> > 
> > I dont have strong opinions about wrappers, sometimes it seems
> > appropriate, sometimes it does not.. tell me the guidance here
> > and I'll kick the patch to fit ;-)
> 
> Well, a wrapper like perf_data_file__is_pipe(file_out) that maps to
> file_out->is_pipe and will produce the same results at every call and
> that we don't have the slightest intention of somehow hooking, I would
> do away with it and use file_out->is_pipe directly.

ok

thanks,
jirka
--
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/