Messages in this thread | | | Date | Tue, 26 Nov 2013 14:07:13 +0100 | From | Jiri Olsa <> | 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
| |