Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200608161805.65841-1-eranian@google.com>
In-Reply-To: <20200608161805.65841-1-eranian@google.com>
From:   Ian Rogers <irogers@google.com>
Date:   Mon, 8 Jun 2020 09:52:43 -0700
Message-ID: <CAP-5=fXmrbBqDj6udGJCLHF5ePERr1S5qTKGZZAUBC1EmA-8LQ@mail.gmail.com>
Subject: Re: [PATCH] perf headers: fix processing of pmu_mappings
To:     Stephane Eranian <eranian@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>, mingo@elte.hu
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 8, 2020 at 9:18 AM Stephane Eranian <eranian@google.com> wrote:
>
> This patch fixes a bug in process_pmu_mappings() where the code
> would not produce an env->pmu_mappings string that was easily parsable.
> The function parses the PMU_MAPPING header information into a string
> consisting of value:name pairs where value is the PMU type identifier
> and name is the PMU name, e.g., 10:ibs_fetch. As it was, the code
> was producing a truncated string with only the first pair showing
> even though the rest was there but after the \0.
> This patch fixes the problem byt adding a proper white space between
> pairs and moving the \0 termination to the end. With this patch applied,
> all pairs appear and are easily parsed.
>
> Before:
> 14:amd_iommu_1
>
> After:
> 14:amd_iommu_1 7:uprobe 5:breakpoint 10:amd_l3 19:amd_iommu_6 8:power 4:cpu 17:amd_iommu_4 15:amd_iommu_2 1:software 6:kprobe 13:amd_iommu_0 9:amd_df 20:amd_iommu_7 18:amd_iommu_5 2:tracepoint 21:msr 12:ibs_op 16:amd_iommu_3 11:ibs_fetch
>
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  tools/perf/util/header.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 7a67d017d72c3..cf72124da9350 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2462,13 +2462,15 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
>  static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>  {
>         char *name;
> -       u32 pmu_num;
> +       u32 pmu_num, o_num;
>         u32 type;
>         struct strbuf sb;
>
>         if (do_read_u32(ff, &pmu_num))
>                 return -1;
>
> +       o_num = pmu_num;
> +
>         if (!pmu_num) {
>                 pr_debug("pmu mappings not available\n");
>                 return 0;
> @@ -2486,10 +2488,11 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>                 if (!name)
>                         goto error;
>
> -               if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
> +               /* add proper spacing between entries */
> +               if (pmu_num < o_num && strbuf_add(&sb, " ", 1) < 0)
>                         goto error;
> -               /* include a NULL character at the end */
> -               if (strbuf_add(&sb, "", 1) < 0)
> +
> +               if (strbuf_addf(&sb, "%u:%s", type, name) < 0)
>                         goto error;
>
>                 if (!strcmp(name, "msr"))
> @@ -2498,6 +2501,9 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
>                 free(name);
>                 pmu_num--;
>         }
> +       /* include a NULL character at the end */
> +       if (strbuf_add(&sb, "", 1) < 0)
> +               goto error;
>         ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
>         return 0;
>
> --
> 2.27.0.278.ge193c7cf3a9-goog

Acked-by: Ian Rogers <irogers@google.com>

A lot of the complexity in this code came from strbuf not \0
terminating strings. Would a strbuf that always \0 terminated be a
useful change? In general there's a lack of consistent style with
strbuf, strcat and asprintf being used in different parts of the code.
Perhaps strbuf use should migrate to asprintf? There are currently
just 13 callers of strbuf_init.

Thanks,
Ian