linux-kernel - Re: [PATCH 2/3] perf list: Get rid of json_print

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fUkaNybog6ojcatuyQ2+zBD-BZXE-rYZkHKP91VoT0P=g@mail.gmail.com>
Date: Wed, 19 Nov 2025 08:40:13 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, James Clark <james.clark@...aro.org>, 
	Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, 
	LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 2/3] perf list: Get rid of json_print_state

On Mon, Nov 17, 2025 at 10:36 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> The JSON print state has only one different field (need_sep).  Let's
> move it to the default print state and use it.  It would waste a little
> bit of more spaces but who cares? :)
>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  tools/perf/builtin-list.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 28bf1fc7f5eeff8f..1ab969ffe371c5cb 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -60,6 +60,8 @@ struct print_state {
>         bool metricgroups;
>         /** @exclude_abi: Exclude PMUs with types less than PERF_TYPE_MAX except PERF_TYPE_RAW. */
>         bool exclude_abi;
> +       /** @need_sep: Should a separator be printed prior to the next item? (for JSON) */
> +       bool need_sep;

I'm concerned we might be reinventing something like the stat-display code :-)

Perhaps this would be cleaner as:
```
struct json_print_state {
  /** State for regular filtering, etc. */
  struct print_state ps;
  /** Should a json separator ',' be printed prior to the next item? */
  bool need_sep;
}
```
So we have the regular print state stuff in the json print_state but
don't expose json-ness things to other print_state users - and
hopefully avoid convoluted shared state, function pointers, etc.

Thanks,
Ian

>         /** @last_topic: The last printed event topic. */
>         char *last_topic;
>         /** @last_metricgroups: The last printed metric group. */
> @@ -368,7 +370,7 @@ static void json_print_event(void *ps, const char *topic,
>                              const char *desc, const char *long_desc,
>                              const char *encoding_desc)
>  {
> -       struct json_print_state *print_state = ps;
> +       struct print_state *print_state = ps;
>         bool need_sep = false;
>         FILE *fp = print_state->fp;
>         struct strbuf buf;
> @@ -444,7 +446,7 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
>                               const char *threshold, const char *unit,
>                               const char *pmu_name)
>  {
> -       struct json_print_state *print_state = ps;
> +       struct print_state *print_state = ps;
>         bool need_sep = false;
>         FILE *fp = print_state->fp;
>         struct strbuf buf;
> @@ -521,9 +523,6 @@ int cmd_list(int argc, const char **argv)
>                 .fp = stdout,
>                 .desc = true,
>         };
> -       struct print_state json_ps = {
> -               .fp = stdout,
> -       };
>         void *ps = &default_ps;
>         struct print_callbacks print_cb = {
>                 .print_start = default_print_start,
> @@ -574,7 +573,6 @@ int cmd_list(int argc, const char **argv)
>
>         if (output_path) {
>                 default_ps.fp = fopen(output_path, "w");
> -               json_ps.fp = default_ps.fp;
>         }
>
>         setup_pager();
> @@ -590,7 +588,7 @@ int cmd_list(int argc, const char **argv)
>                         .print_metric = json_print_metric,
>                         .skip_duplicate_pmus = json_skip_duplicate_pmus,
>                 };
> -               ps = &json_ps;
> +               ps = &default_ps;
>         } else {
>                 default_ps.last_topic = strdup("");
>                 assert(default_ps.last_topic);
> --
> 2.52.0.rc1.455.g30608eb744-goog
>