Re: [PATCH 2/5] perf annotate: use run-command.h to fork objdump

[Ian Rogers]

Thanks for applying! Apologies, there was a missing null termination.
Sent in a follow-up patch.

Ian

On Mon, Oct 14, 2019 at 7:18 AM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Thu, Oct 10, 2019 at 11:36:46AM -0700, Ian Rogers escreveu:
> > Reduce duplicated logic by using the subcmd library. Ensure when errors
> > occur they are reported to the caller. Before this patch, if no lines are
> > read the error status is 0.
>
> Thanks, applied and tested.
>
> - Arnaldo
>
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/util/annotate.c | 71 +++++++++++++++++++-------------------
> >  1 file changed, 36 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 1487849a191a..fc12c5cfe112 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -43,6 +43,7 @@
> >  #include <linux/string.h>
> >  #include <bpf/libbpf.h>
> >  #include <subcmd/parse-options.h>
> > +#include <subcmd/run-command.h>
> >
> >  /* FIXME: For the HE_COLORSET */
> >  #include "ui/browser.h"
> > @@ -1843,12 +1844,18 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >       struct kcore_extract kce;
> >       bool delete_extract = false;
> >       bool decomp = false;
> > -     int stdout_fd[2];
> >       int lineno = 0;
> >       int nline;
> > -     pid_t pid;
> >       char *line;
> >       size_t line_len;
> > +     const char *objdump_argv[] = {
> > +             "/bin/sh",
> > +             "-c",
> > +             NULL, /* Will be the objdump command to run. */
> > +             "--",
> > +             NULL, /* Will be the symfs path. */
> > +     };
> > +     struct child_process objdump_process;
> >       int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
> >
> >       if (err)
> > @@ -1878,7 +1885,7 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >
> >               if (dso__decompress_kmodule_path(dso, symfs_filename,
> >                                                tmp, sizeof(tmp)) < 0)
> > -                     goto out;
> > +                     return -1;
> >
> >               decomp = true;
> >               strcpy(symfs_filename, tmp);
> > @@ -1903,38 +1910,28 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >
> >       pr_debug("Executing: %s\n", command);
> >
> > -     err = -1;
> > -     if (pipe(stdout_fd) < 0) {
> > -             pr_err("Failure creating the pipe to run %s\n", command);
> > -             goto out_free_command;
> > -     }
> > -
> > -     pid = fork();
> > -     if (pid < 0) {
> > -             pr_err("Failure forking to run %s\n", command);
> > -             goto out_close_stdout;
> > -     }
> > +     objdump_argv[2] = command;
> > +     objdump_argv[4] = symfs_filename;
> >
> > -     if (pid == 0) {
> > -             close(stdout_fd[0]);
> > -             dup2(stdout_fd[1], 1);
> > -             close(stdout_fd[1]);
> > -             execl("/bin/sh", "sh", "-c", command, "--", symfs_filename,
> > -                   NULL);
> > -             perror(command);
> > -             exit(-1);
> > +     /* Create a pipe to read from for stdout */
> > +     memset(&objdump_process, 0, sizeof(objdump_process));
> > +     objdump_process.argv = objdump_argv;
> > +     objdump_process.out = -1;
> > +     if (start_command(&objdump_process)) {
> > +             pr_err("Failure starting to run %s\n", command);
> > +             err = -1;
> > +             goto out_free_command;
> >       }
> >
> > -     close(stdout_fd[1]);
> > -
> > -     file = fdopen(stdout_fd[0], "r");
> > +     file = fdopen(objdump_process.out, "r");
> >       if (!file) {
> >               pr_err("Failure creating FILE stream for %s\n", command);
> >               /*
> >                * If we were using debug info should retry with
> >                * original binary.
> >                */
> > -             goto out_free_command;
> > +             err = -1;
> > +             goto out_close_stdout;
> >       }
> >
> >       /* Storage for getline. */
> > @@ -1958,8 +1955,14 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >       }
> >       free(line);
> >
> > -     if (nline == 0)
> > +     err = finish_command(&objdump_process);
> > +     if (err)
> > +             pr_err("Error running %s\n", command);
> > +
> > +     if (nline == 0) {
> > +             err = -1;
> >               pr_err("No output from %s\n", command);
> > +     }
> >
> >       /*
> >        * kallsyms does not have symbol sizes so there may a nop at the end.
> > @@ -1969,23 +1972,21 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
> >               delete_last_nop(sym);
> >
> >       fclose(file);
> > -     err = 0;
> > +
> > +out_close_stdout:
> > +     close(objdump_process.out);
> > +
> >  out_free_command:
> >       free(command);
> > -out_remove_tmp:
> > -     close(stdout_fd[0]);
> >
> > +out_remove_tmp:
> >       if (decomp)
> >               unlink(symfs_filename);
> >
> >       if (delete_extract)
> >               kcore_extract__delete(&kce);
> > -out:
> > -     return err;
> >
> > -out_close_stdout:
> > -     close(stdout_fd[1]);
> > -     goto out_free_command;
> > +     return err;
> >  }
> >
> >  static void calc_percent(struct sym_hist *sym_hist,
> > --
> > 2.23.0.581.g78d2f28ef7-goog
>
> --
>
> - Arnaldo