lkml.org 
[lkml]   [2013]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2013-11-26 14:41    [W:0.082 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site