Arnaldo Carvalho de Melo wrote:
> Em Thu, Dec 01, 2016 at 01:04:36AM +0100, Alexis Berlemont escreveu:
> > Before disassembling, the tool objdump is called just to be sure:
> > * objdump is available in the path;
> > * objdump is an executable binary;
> > * objdump has no dependency issue or anything else.
> >
> > This objdump "pre-"command is only necessary because the real objdump
> > command is followed by some " | grep ..."; this prevents the shell
> > from returning the exit code of objdump execution.
> >
> > Signed-off-by: Alexis Berlemont <alexis.berlem...@gmail.com>
> > ---
> > tools/perf/util/annotate.c | 79
> > +++++++++++++++++++++++++++++++++++++++++++++-
> > tools/perf/util/annotate.h | 3 ++
> > 2 files changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 3e34ee0..9d6c3a0 100644
> > --- a/tools/perf/util/annotate.c
>
> > +static int annotate__check_objdump(void)
> > +{
> > + char command[PATH_MAX * 2];
> > + int wstatus, err;
> > + pid_t pid;
> > +
> > + snprintf(command, sizeof(command),
> > + "%s -v > /dev/null 2>&1",
> > + objdump_path ? objdump_path : "objdump");
> > +
> > + pid = fork();
> > + if (pid < 0) {
> > + pr_err("Failure forking to run %s\n", command);
> > + return -1;
> > + }
> > +
> > + if (pid == 0) {
> > + execl("/bin/sh", "sh", "-c", command, NULL);
> > + exit(-1);
> > + }
> > +
> > + err = waitpid(pid, &wstatus, 0);
> > + if (err < 0) {
> > + pr_err("Failure calling waitpid: %s: (%s)\n",
> > + strerror(errno), command);
> > + return -1;
> > + }
> > +
> > + pr_err("%s: %d %d\n", command, pid, WEXITSTATUS(wstatus));
>
> So this will appear in all cases, no need for that, i.e. in the success
> case we don't need to get that flashing on the screen, on the last line.
>
Many thanks for your answer and your time.
Sorry for the late anwser and such an obvious error.
> > + switch (WEXITSTATUS(wstatus)) {
> > + case 0:
> > + /* Success */
> > + err = 0;
> > + break;
>
> So probably you want to return 0; here instead and then at the error
> case, i.e. when you set err to !0 you do that pr_err() call above, but I
> think it would be better to use pr_debug(), the warning on the popup box
> is what by default is more polished to tell the user, the details are
> for developers or people wanting to dig in.
>
> But while doing this I thought that you could instead call this only
> after objdump fails, i.e. if all is right, no need for checking what
> went wrong.
>
> I.e. you would do the grep step separately, after checking objdump's
> error.
>
> If you think that is too much work, then please just do the
> pr_err->pr_debug conversion, which would remove the flashing for the
> success case.
I will do the grep separately; no problem.
Alexis.
>
> I tested it, btw, using:
>
> perf annotate --objdump /dev/null page_fault
>
> Which produced a better output than what we have now (nothing):
>
>
> ??????Error:????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
> ???Couldn't annotate page_fault: ???
> ???The objdump tool found in $PATH cannot be executed???
> ??? ???
> ??? ???
> ???Press any key... ???
>
> ????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
>
>
>
> /dev/null -v > /dev/null 2>&1: 10336 126
>
>
> -------------------
>
> summary: make that last line appear only when -v is used (pr_debug) and
> consider covering the case where --objdump was used, where talking about $PATH
> is misleading.
>
>
> > + case 127:
> > + /* The shell did not find objdump in the path */
> > + err = SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP;
> > + break;
> > + default:
> > + /*
> > + * In the default case, we consider that objdump
> > + * cannot be executed; so it gathers many fault
> > + * scenarii:
> > + * - objdump is not an executable (126);
> > + * - objdump has some dependency issue;
> > + * - ...
> > + */
> > + err = SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP;
> > + break;
> > + }
> > +
> > + return err;
> > +}
> > +
> > static const char *annotate__norm_arch(const char *arch_name)
> > {
> > struct utsname uts;
> > @@ -1351,6 +1424,10 @@ int symbol__disassemble(struct symbol *sym, struct
> > map *map, const char *arch_na
> > if (err)
> > return err;
> >
> > + err = annotate__check_objdump();
> > + if (err)
> > + return err;
> > +
> > arch_name = annotate__norm_arch(arch_name);
> > if (!arch_name)
> > return -1;
> > @@ -1482,7 +1559,7 @@ int symbol__disassemble(struct symbol *sym, struct
> > map *map, const char *arch_na
> > delete_last_nop(sym);
> >
> > fclose(file);
> > - err = 0;
> > + err = nline == 0 ? SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT : 0;
> > out_remove_tmp:
> > close(stdout_fd[0]);
> >
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 87e4cad..123f60c 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -172,6 +172,9 @@ enum symbol_disassemble_errno {
> > __SYMBOL_ANNOTATE_ERRNO__START = -10000,
> >
> > SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX =
> > __SYMBOL_ANNOTATE_ERRNO__START,
> > + SYMBOL_ANNOTATE_ERRNO__NO_EXEC_OBJDUMP,
> > + SYMBOL_ANNOTATE_ERRNO__NO_OBJDUMP,
> > + SYMBOL_ANNOTATE_ERRNO__NO_OUTPUT,
> >
> > __SYMBOL_ANNOTATE_ERRNO__END,
> > };
> > --
> > 2.10.2
