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
>
> > .path = inject->input_name,
> > .mode = PERF_DATA_MODE_READ,
> > };
> > + struct perf_data_file *file_out = &inject->output;
> > + int out_fd = perf_data_file__fd(file_out);
> >
> > signal(SIGINT, sig_handler);
> >
> > @@ -365,7 +359,7 @@ static int __cmd_inject(struct perf_inject *inject)
> > inject->tool.tracing_data = perf_event__repipe_tracing_data;
> > }
> >
> > - session = perf_session__new(&file, true, &inject->tool);
> > + session = perf_session__new(&file_in, true, &inject->tool);
>
> This hunk, for example, wouldn't be here, the this patch would be
> shorter, easier to review.
>
> > if (session == NULL)
> > return -ENOMEM;
> >
> > @@ -391,14 +385,15 @@ static int __cmd_inject(struct perf_inject *inject)
> > }
> > }
> >
> > - if (!inject->pipe_output)
> > - lseek(inject->output, session->header.data_offset, SEEK_SET);
> > + if (!perf_data_file__is_pipe(file_out))
> > + lseek(out_fd, session->header.data_offset, SEEK_SET);
>
> Couldn't this be left as:
>
> - if (!inject->pipe_output)
> - lseek(inject->output, session->header.data_offset, SEEK_SET);
> + if (!perf_data_file__is_pipe(file_out))
> + lseek(inject->output->fd, session->header.data_offset,
> SEEK_SET);
>
> I.e. why wrap access to the fd like that?
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
>
> >
> > ret = perf_session__process_events(session, &inject->tool);
> >
> > - 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, out_fd,
> > + true);
>
> Why a line for 'true' all by itself?
line was crossing 80 chars limit
>
> > }
> >
> > perf_session__delete(session);
> > @@ -427,14 +422,17 @@ int cmd_inject(int argc, const char **argv, const
> > char *prefix __maybe_unused)
> > },
> > .input_name = "-",
> > .samples = LIST_HEAD_INIT(inject.samples),
> > + .output = {
> > + .path = "-",
> > + .mode = PERF_DATA_MODE_WRITE,
> > + },
> > };
> > - const char *output_name = "-";
> > const struct option options[] = {
> > OPT_BOOLEAN('b', "build-ids", &inject.build_ids,
> > "Inject build-ids into the output stream"),
> > OPT_STRING('i', "input", &inject.input_name, "file",
> > "input file name"),
> > - 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 ;-)
thanks,
jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
