Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753122AbcLFTiO (ORCPT ); Tue, 6 Dec 2016 14:38:14 -0500 Received: from mail.kernel.org ([198.145.29.136]:39586 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751166AbcLFTiM (ORCPT ); Tue, 6 Dec 2016 14:38:12 -0500 Date: Tue, 6 Dec 2016 16:38:05 -0300 From: Arnaldo Carvalho de Melo To: Alexis Berlemont Cc: linux-kernel@vger.kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com Subject: Re: [PATCH] perf annotate: check that objdump correctly works Message-ID: <20161206193805.GB8257@kernel.org> References: <20161201000436.10354-1-alexis.berlemont@gmail.com> <20161201000436.10354-2-alexis.berlemont@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161201000436.10354-2-alexis.berlemont@gmail.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5138 Lines: 161 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 > --- > 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. > + 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 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