Re: [PATCH v2 2/7] perf: Use perf_pmu__open_file() and perf_pmu__scan_file()

On 23/12/2022 03:45, Leo Yan wrote:

On Thu, Dec 22, 2022 at 04:03:22PM +0000, James Clark wrote:

...

Remove some code that duplicates existing methods. This requires that some consts are removed because one of the existing helper methods takes a struct perf_pmu instead of a name which has a non const name field. But except for the tests, the strings were already non const.

No functional changes.

Signed-off-by: James Clark james.clark@arm.com

tools/perf/tests/evsel-roundtrip-name.c | 3 +- tools/perf/tests/parse-events.c | 3 +- tools/perf/util/cputopo.c | 9 +----- tools/perf/util/pmu-hybrid.c | 27 +++------------- tools/perf/util/pmu-hybrid.h | 2 +- tools/perf/util/pmu.c | 42 +++++++------------------ tools/perf/util/pmu.h | 3 +- 7 files changed, 24 insertions(+), 65 deletions(-)

diff --git a/tools/perf/tests/evsel-roundtrip-name.c b/tools/perf/tests/evsel-roundtrip-name.c index e94fed901992..9bccf177f481 100644 --- a/tools/perf/tests/evsel-roundtrip-name.c +++ b/tools/perf/tests/evsel-roundtrip-name.c @@ -103,8 +103,9 @@ static int test__perf_evsel__roundtrip_name_test(struct test_suite *test __maybe int subtest __maybe_unused) { int err = 0, ret = 0;

• char cpu_atom[] = "cpu_atom";

• if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted("cpu_atom"))

• if (perf_pmu__has_hybrid() && perf_pmu__hybrid_mounted(cpu_atom))

After change the parameter 'name' to non const type in function perf_pmu__hybrid_mounted(), at here we still can pass string "cpu_atom" without warning, right? If so, we don't need to define a local variable 'cpu_atom'.

[...]

...

-bool perf_pmu__hybrid_mounted(const char *name) +bool perf_pmu__hybrid_mounted(char *name) {

• char path[PATH_MAX];
• const char *sysfs;
• FILE *file;
• int n, cpu;

• int cpu;
• struct perf_pmu pmu = {.name = name};

if (strncmp(name, "cpu_", 4)) return false;

• sysfs = sysfs__mountpoint();
• if (!sysfs)

• return false;
  

• snprintf(path, PATH_MAX, CPUS_TEMPLATE_CPU, sysfs, name);
• if (!file_available(path))

• return false;
  

• file = fopen(path, "r");
• if (!file)

• return false;
  

• n = fscanf(file, "%u", &cpu);
• fclose(file);
• if (n <= 0)

• return false;
  

• return true;

• return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0;

It's not necessarily to change the parameter 'name' to non const type, we can handle this case in two different ways.

The first option is to refine the code with using the function perf_pmu__pathname_scnprintf() which is introduced in patch 01, thus we can keep using fopen() and fscanf() to read out value from 'cpus' entry.

Another option is to define a local array 'pmu_name[PATH_MAX]' and then copy the input string to this array, something like:

bool perf_pmu__hybrid_mounted(const char *name) { char pmu_name[PATH_MAX]; struct perf_pmu pmu; int cpu;

strncpy(pmu_name, name, sizeof(pmu_name)); pmu.name = pmu_name;

return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; }

We even can consider to dynamically allocate buffer and copy string from 'name' to the allocated buffer.

I went with the string copy way and put the consts back in V3. One minor difference is that I had to use strlcpy() instead of strncpy() to avoid a compiler warning about the destination buffer being the same size.

James

[...]

...

-static struct perf_cpu_map *pmu_cpumask(const char *name) +static struct perf_cpu_map *pmu_cpumask(char *name) {

• char path[PATH_MAX]; struct perf_cpu_map *cpus;
• const char *sysfs = sysfs__mountpoint(); const char *templates[] = {

• CPUS_TEMPLATE_UNCORE,
  

• CPUS_TEMPLATE_CPU,
  

• "cpumask",
  

• "cpus",
  

  NULL }; const char **template;

• if (!sysfs)

• return NULL;
  

• struct perf_pmu pmu = {.name = name};

Here we also can define a local array and copy string from 'name' to the local array. This can allow us to provide a friendly function and caller doesn't need to explictly pass non const string.

Done