Re: [PATCH 6/8] perf annotate: Ensure init/exit for global options
From: Ian Rogers
Date: Tue Nov 28 2023 - 14:21:19 EST
On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Now it only cares about the global options so it can just handle it
> without the argument.
If annotate_opts were accessed by a function then you could
pthread_once the initialization on the first call to get
annotate_opts. Removing annotation_options__init/exit would remove
some potential for error.
Thanks,
Ian
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
> tools/perf/builtin-annotate.c | 8 ++++----
> tools/perf/builtin-report.c | 8 ++++----
> tools/perf/builtin-top.c | 8 ++++----
> tools/perf/util/annotate.c | 19 +++++++++++--------
> tools/perf/util/annotate.h | 8 ++++----
> 5 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 87af95634879..9b3dd456a1ee 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -616,13 +616,13 @@ int cmd_annotate(int argc, const char **argv)
> set_option_flag(options, 0, "show-total-period", PARSE_OPT_EXCLUSIVE);
> set_option_flag(options, 0, "show-nr-samples", PARSE_OPT_EXCLUSIVE);
>
> - annotation_options__init(&annotate_opts);
> + annotation_options__init();
>
> ret = hists__init();
> if (ret < 0)
> return ret;
>
> - annotation_config__init(&annotate_opts);
> + annotation_config__init();
>
> argc = parse_options(argc, argv, options, annotate_usage, 0);
> if (argc) {
> @@ -652,7 +652,7 @@ int cmd_annotate(int argc, const char **argv)
> return -ENOMEM;
> }
>
> - if (annotate_check_args(&annotate_opts) < 0)
> + if (annotate_check_args() < 0)
> return -EINVAL;
>
> #ifdef HAVE_GTK2_SUPPORT
> @@ -733,7 +733,7 @@ int cmd_annotate(int argc, const char **argv)
> #ifndef NDEBUG
> perf_session__delete(annotate.session);
> #endif
> - annotation_options__exit(&annotate_opts);
> + annotation_options__exit();
>
> return ret;
> }
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index bc0d986c1e0c..17fb171e898b 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1430,7 +1430,7 @@ int cmd_report(int argc, const char **argv)
> */
> symbol_conf.keep_exited_threads = true;
>
> - annotation_options__init(&annotate_opts);
> + annotation_options__init();
>
> ret = perf_config(report__config, &report);
> if (ret)
> @@ -1464,7 +1464,7 @@ int cmd_report(int argc, const char **argv)
> return -ENOMEM;
> }
>
> - if (annotate_check_args(&annotate_opts) < 0) {
> + if (annotate_check_args() < 0) {
> ret = -EINVAL;
> goto exit;
> }
> @@ -1696,7 +1696,7 @@ int cmd_report(int argc, const char **argv)
> */
> symbol_conf.priv_size += sizeof(u32);
> }
> - annotation_config__init(&annotate_opts);
> + annotation_config__init();
> }
>
> if (symbol__init(&session->header.env) < 0)
> @@ -1750,7 +1750,7 @@ int cmd_report(int argc, const char **argv)
> zstd_fini(&(session->zstd_data));
> perf_session__delete(session);
> exit:
> - annotation_options__exit(&annotate_opts);
> + annotation_options__exit();
> free(sort_order_help);
> free(field_order_help);
> return ret;
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 60399e4233ee..0de963ca3196 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1608,7 +1608,7 @@ int cmd_top(int argc, const char **argv)
> if (status < 0)
> return status;
>
> - annotation_options__init(&annotate_opts);
> + annotation_options__init();
>
> annotate_opts.min_pcnt = 5;
> annotate_opts.context = 4;
> @@ -1660,7 +1660,7 @@ int cmd_top(int argc, const char **argv)
> if (status)
> goto out_delete_evlist;
>
> - if (annotate_check_args(&annotate_opts) < 0)
> + if (annotate_check_args() < 0)
> goto out_delete_evlist;
>
> if (!top.evlist->core.nr_entries) {
> @@ -1786,7 +1786,7 @@ int cmd_top(int argc, const char **argv)
> if (status < 0)
> goto out_delete_evlist;
>
> - annotation_config__init(&annotate_opts);
> + annotation_config__init();
>
> symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
> status = symbol__init(NULL);
> @@ -1839,7 +1839,7 @@ int cmd_top(int argc, const char **argv)
> out_delete_evlist:
> evlist__delete(top.evlist);
> perf_session__delete(top.session);
> - annotation_options__exit(&annotate_opts);
> + annotation_options__exit();
>
> return status;
> }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index daff9af552f4..626ff3baeb85 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -3416,8 +3416,10 @@ static int annotation__config(const char *var, const char *value, void *data)
> return 0;
> }
>
> -void annotation_options__init(struct annotation_options *opt)
> +void annotation_options__init(void)
> {
> + struct annotation_options *opt = &annotate_opts;
> +
> memset(opt, 0, sizeof(*opt));
>
> /* Default values. */
> @@ -3428,16 +3430,15 @@ void annotation_options__init(struct annotation_options *opt)
> opt->percent_type = PERCENT_PERIOD_LOCAL;
> }
>
> -
> -void annotation_options__exit(struct annotation_options *opt)
> +void annotation_options__exit(void)
> {
> - zfree(&opt->disassembler_style);
> - zfree(&opt->objdump_path);
> + zfree(&annotate_opts.disassembler_style);
> + zfree(&annotate_opts.objdump_path);
> }
>
> -void annotation_config__init(struct annotation_options *opt)
> +void annotation_config__init(void)
> {
> - perf_config(annotation__config, opt);
> + perf_config(annotation__config, &annotate_opts);
> }
>
> static unsigned int parse_percent_type(char *str1, char *str2)
> @@ -3491,8 +3492,10 @@ int annotate_parse_percent_type(const struct option *opt __maybe_unused, const c
> return err;
> }
>
> -int annotate_check_args(struct annotation_options *args)
> +int annotate_check_args(void)
> {
> + struct annotation_options *args = &annotate_opts;
> +
> if (args->prefix_strip && !args->prefix) {
> pr_err("--prefix-strip requires --prefix\n");
> return -1;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 857c5fa0e6b1..4283eb4522b2 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -428,14 +428,14 @@ static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
> }
> #endif
>
> -void annotation_options__init(struct annotation_options *opt);
> -void annotation_options__exit(struct annotation_options *opt);
> +void annotation_options__init(void);
> +void annotation_options__exit(void);
>
> -void annotation_config__init(struct annotation_options *opt);
> +void annotation_config__init(void);
>
> int annotate_parse_percent_type(const struct option *opt, const char *_str,
> int unset);
>
> -int annotate_check_args(struct annotation_options *args);
> +int annotate_check_args(void);
>
> #endif /* __PERF_ANNOTATE_H */
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>