Perf inject was missing the --vmlinux argument so I've added it. At
the same time I tidied up the argument checking that already existed in
perf report and applied it to all tools.
What I'm not sure about is whether it would have been better
to check the accessibility of these files further down in a common place?
It seems like vmlinux_name is also used for a non user provided name at
some point so maybe this would be more complicated than just applying the
existing check everywhere.
Thanks
James
James Clark (3):
perf tools: Refactor out kernel symbol argument sanity checking
perf tools: Check vmlinux/kallsyms arguments in all tools
perf inject: Add vmlinux and ignore-vmlinux arguments
tools/perf/builtin-annotate.c | 4 ++++
tools/perf/builtin-c2c.c | 4 ++++
tools/perf/builtin-inject.c | 7 +++++++
tools/perf/builtin-probe.c | 5 +++++
tools/perf/builtin-record.c | 4 ++++
tools/perf/builtin-report.c | 13 ++-----------
tools/perf/builtin-sched.c | 4 ++++
tools/perf/builtin-script.c | 3 +++
tools/perf/builtin-top.c | 4 ++++
tools/perf/util/symbol.c | 22 ++++++++++++++++++++++
tools/perf/util/symbol.h | 2 ++
11 files changed, 61 insertions(+), 11 deletions(-)
--
2.28.0
Only perf report checked the validity of these arguments so apply the
same check to all tools that read them for consistency.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/builtin-annotate.c | 4 ++++
tools/perf/builtin-c2c.c | 4 ++++
tools/perf/builtin-probe.c | 5 +++++
tools/perf/builtin-record.c | 4 ++++
tools/perf/builtin-sched.c | 4 ++++
tools/perf/builtin-script.c | 3 +++
tools/perf/builtin-top.c | 4 ++++
7 files changed, 28 insertions(+)
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 05eb098cb0e3..490bb9b8cf17 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -591,6 +591,10 @@ int cmd_annotate(int argc, const char **argv)
return ret;
}
+ ret = symbol__validate_sym_arguments();
+ if (ret)
+ return ret;
+
if (quiet)
perf_quiet_option();
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index a192014fa52b..b5c67ef73862 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2768,6 +2768,10 @@ static int perf_c2c__report(int argc, const char **argv)
if (c2c.stats_only)
c2c.use_stdio = true;
+ err = symbol__validate_sym_arguments();
+ if (err)
+ goto out;
+
if (!input_name || !strlen(input_name))
input_name = "perf.data";
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index e1dd51f2874b..c31627af75d4 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -21,6 +21,7 @@
#include "util/build-id.h"
#include "util/strlist.h"
#include "util/strfilter.h"
+#include "util/symbol.h"
#include "util/symbol_conf.h"
#include "util/debug.h"
#include <subcmd/parse-options.h>
@@ -629,6 +630,10 @@ __cmd_probe(int argc, const char **argv)
params.command = 'a';
}
+ ret = symbol__validate_sym_arguments();
+ if (ret)
+ return ret;
+
if (params.quiet) {
if (verbose != 0) {
pr_err(" Error: -v and -q are exclusive.\n");
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 41bb884f5a74..9aff49915148 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2712,6 +2712,10 @@ int cmd_record(int argc, const char **argv)
if (quiet)
perf_quiet_option();
+ err = symbol__validate_sym_arguments();
+ if (err)
+ return err;
+
/* Make system wide (-a) the default target. */
if (!argc && target__none(&rec->opts.target))
rec->opts.target.system_wide = true;
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 635a6b5a9ec9..4527f632ebe4 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3538,6 +3538,7 @@ int cmd_sched(int argc, const char **argv)
.fork_event = replay_fork_event,
};
unsigned int i;
+ int ret;
for (i = 0; i < ARRAY_SIZE(sched.curr_pid); i++)
sched.curr_pid[i] = -1;
@@ -3598,6 +3599,9 @@ int cmd_sched(int argc, const char **argv)
parse_options_usage(NULL, timehist_options, "n", true);
return -EINVAL;
}
+ ret = symbol__validate_sym_arguments();
+ if (ret)
+ return ret;
return perf_sched__timehist(&sched);
} else {
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 6211d0b84b7a..53f8f7b408be 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3836,6 +3836,9 @@ int cmd_script(int argc, const char **argv)
data.path = input_name;
data.force = symbol_conf.force;
+ if (symbol__validate_sym_arguments())
+ return -1;
+
if (argc > 1 && !strncmp(argv[0], "rec", strlen("rec"))) {
rec_script_path = get_script_path(argv[1], RECORD_SUFFIX);
if (!rec_script_path)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 020c4f110c10..1fc390f136dd 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1618,6 +1618,10 @@ int cmd_top(int argc, const char **argv)
if (argc)
usage_with_options(top_usage, options);
+ status = symbol__validate_sym_arguments();
+ if (status)
+ goto out_delete_evlist;
+
if (annotate_check_args(&top.annotation_opts) < 0)
goto out_delete_evlist;
--
2.28.0
User supplied values for vmlinux and kallsyms are checked before
continuing. Refactor this into a function so that it can be used
elsewhere.
Signed-off-by: James Clark <[email protected]>
---
tools/perf/builtin-report.c | 13 ++-----------
tools/perf/util/symbol.c | 22 ++++++++++++++++++++++
tools/perf/util/symbol.h | 2 ++
3 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index a0316ce910db..8167ebfe776a 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1378,18 +1378,9 @@ int cmd_report(int argc, const char **argv)
if (quiet)
perf_quiet_option();
- if (symbol_conf.vmlinux_name &&
- access(symbol_conf.vmlinux_name, R_OK)) {
- pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
- ret = -EINVAL;
- goto exit;
- }
- if (symbol_conf.kallsyms_name &&
- access(symbol_conf.kallsyms_name, R_OK)) {
- pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
- ret = -EINVAL;
+ ret = symbol__validate_sym_arguments();
+ if (ret)
goto exit;
- }
if (report.inverted_callchain)
callchain_param.order = ORDER_CALLER;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 0fc9a5410739..8fad1f0d41cb 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2630,3 +2630,25 @@ struct mem_info *mem_info__new(void)
refcount_set(&mi->refcnt, 1);
return mi;
}
+
+/*
+ * Checks that user supplied symbol kernel files are accessible because
+ * the default mechanism for accessing elf files fails silently. i.e. if
+ * debug syms for a build ID aren't found perf carries on normally. When
+ * they are user supplied we should assume that the user doesn't want to
+ * silently fail.
+ */
+int symbol__validate_sym_arguments(void)
+{
+ if (symbol_conf.vmlinux_name &&
+ access(symbol_conf.vmlinux_name, R_OK)) {
+ pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
+ return -EINVAL;
+ }
+ if (symbol_conf.kallsyms_name &&
+ access(symbol_conf.kallsyms_name, R_OK)) {
+ pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
+ return -EINVAL;
+ }
+ return 0;
+}
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 954d6a049ee2..166196686f2e 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -286,4 +286,6 @@ static inline void __mem_info__zput(struct mem_info **mi)
#define mem_info__zput(mi) __mem_info__zput(&mi)
+int symbol__validate_sym_arguments(void);
+
#endif /* __PERF_SYMBOL */
--
2.28.0
On Mon, Oct 18, 2021 at 6:48 AM James Clark <[email protected]> wrote:
>
> User supplied values for vmlinux and kallsyms are checked before
> continuing. Refactor this into a function so that it can be used
> elsewhere.
>
> Signed-off-by: James Clark <[email protected]>
> ---
> tools/perf/builtin-report.c | 13 ++-----------
> tools/perf/util/symbol.c | 22 ++++++++++++++++++++++
> tools/perf/util/symbol.h | 2 ++
> 3 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index a0316ce910db..8167ebfe776a 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1378,18 +1378,9 @@ int cmd_report(int argc, const char **argv)
> if (quiet)
> perf_quiet_option();
>
> - if (symbol_conf.vmlinux_name &&
> - access(symbol_conf.vmlinux_name, R_OK)) {
> - pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
> - ret = -EINVAL;
> - goto exit;
> - }
> - if (symbol_conf.kallsyms_name &&
> - access(symbol_conf.kallsyms_name, R_OK)) {
> - pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
> - ret = -EINVAL;
> + ret = symbol__validate_sym_arguments();
> + if (ret)
> goto exit;
> - }
>
> if (report.inverted_callchain)
> callchain_param.order = ORDER_CALLER;
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 0fc9a5410739..8fad1f0d41cb 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2630,3 +2630,25 @@ struct mem_info *mem_info__new(void)
> refcount_set(&mi->refcnt, 1);
> return mi;
> }
> +
> +/*
> + * Checks that user supplied symbol kernel files are accessible because
> + * the default mechanism for accessing elf files fails silently. i.e. if
> + * debug syms for a build ID aren't found perf carries on normally. When
> + * they are user supplied we should assume that the user doesn't want to
> + * silently fail.
> + */
> +int symbol__validate_sym_arguments(void)
> +{
> + if (symbol_conf.vmlinux_name &&
> + access(symbol_conf.vmlinux_name, R_OK)) {
> + pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
> + return -EINVAL;
> + }
> + if (symbol_conf.kallsyms_name &&
> + access(symbol_conf.kallsyms_name, R_OK)) {
> + pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
> + return -EINVAL;
> + }
> + return 0;
> +}
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 954d6a049ee2..166196686f2e 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -286,4 +286,6 @@ static inline void __mem_info__zput(struct mem_info **mi)
>
> #define mem_info__zput(mi) __mem_info__zput(&mi)
>
> +int symbol__validate_sym_arguments(void);
> +
> #endif /* __PERF_SYMBOL */
> --
> 2.28.0
>
Reviewed-by: Denis Nikitin <[email protected]>