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

Arnaldo Carvalho de Melo
2019-10-14T14:18:36Z

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@google.com>
> ---
> 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