[PATCH v2 0/6] perf kvm: A small update in the default arch event

[PATCH v2 0/6] perf kvm: A small update in the default arch event

[Namhyung Kim]

Hello,

Ian said there's an issue in the perf kvm record command line option parsing.
This is my attempt to address the issue, simplify the code and fix memory leaks.

v2 changes)
 * handle kvm top subcommand  (Sashiko)
 * handle other arch/condition for the default events  (Sashiko)
 * add <stdlib.h> for musl compatibility  (Sashiko)
 * fix some memory leaks

Thanks,
Namhyung


Namhyung Kim (6):
  perf kvm: Factor out kvm_need_default_arch_event()
  perf kvm: Check kvm_need_default_arch_event() early
  perf kvm: Kill STRDUP_FAIL_EXIT()
  perf kvm: Do not copy filename string
  perf kvm: Fix a memory leak in the usage string
  perf test: Simplify perf kvm record/report tests

 tools/perf/builtin-kvm.c                      | 105 ++++++++----------
 tools/perf/tests/shell/kvm.sh                 |   8 +-
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     |  31 +-----
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  |  37 +-----
 tools/perf/util/kvm-stat.c                    |  41 +++++++
 tools/perf/util/kvm-stat.h                    |  18 ++-
 6 files changed, 109 insertions(+), 131 deletions(-)

-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 1/6] perf kvm: Factor out kvm_need_default_arch_event()

[Namhyung Kim]

The kvm_add_default_arch_event() has a similar logic in each arch to
check if there's an existing command line option for events.  Let's
check it in the generic code and remove the duplication.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                      | 17 +++++---
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     | 31 +++-----------
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  | 34 +++------------
 tools/perf/util/kvm-stat.c                    | 41 +++++++++++++++++++
 tools/perf/util/kvm-stat.h                    |  8 ++++
 5 files changed, 70 insertions(+), 61 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 2c6aef1e13a04cf2..2f00cc1fd1c7cdbd 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -51,6 +51,7 @@
 #include <termios.h>
 #include <semaphore.h>
 #include <signal.h>
+#include <stdlib.h>
 #include <math.h>
 #include <perf/mmap.h>
 
@@ -2014,9 +2015,11 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 
 	BUG_ON(i + 2 != rec_argc);
 
-	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
-	if (ret)
-		goto EXIT;
+	if (kvm_need_default_arch_event(EM_HOST, argc, argv)) {
+		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
+		if (ret)
+			goto EXIT;
+	}
 
 	ret = cmd_record(i, rec_argv);
 
@@ -2101,9 +2104,11 @@ static int __cmd_top(int argc, const char **argv)
 
 	BUG_ON(i != argc);
 
-	ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
-	if (ret)
-		goto EXIT;
+	if (kvm_need_default_arch_event(EM_HOST, argc, argv)) {
+		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
+		if (ret)
+			goto EXIT;
+	}
 
 	ret = cmd_top(i, rec_argv);
 
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
index 96d9c4ae020940f0..8d4133c35c12f14b 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
@@ -9,7 +9,6 @@
 
 #include "book3s_hv_exits.h"
 #include "book3s_hcalls.h"
-#include <subcmd/parse-options.h>
 
 #define NR_TPS 4
 
@@ -177,35 +176,15 @@ int __cpu_isa_init_powerpc(struct perf_kvm_stat *kvm)
  */
 int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv)
 {
-	const char **tmp;
-	bool event = false;
-	int i, j = *argc;
+	int j = *argc;
 
-	const struct option event_options[] = {
-		OPT_BOOLEAN('e', "event", &event, NULL),
-		OPT_END()
-	};
-
-	tmp = calloc(j + 1, sizeof(char *));
-	if (!tmp)
+	if (!perf_pmus__have_event("trace_imc", "trace_cycles"))
 		return -EINVAL;
 
-	for (i = 0; i < j; i++)
-		tmp[i] = argv[i];
-
-	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
-	if (!event) {
-		if (perf_pmus__have_event("trace_imc", "trace_cycles")) {
-			argv[j++] = strdup("-e");
-			argv[j++] = strdup("trace_imc/trace_cycles/");
-			*argc += 2;
-		} else {
-			free(tmp);
-			return -EINVAL;
-		}
-	}
+	argv[j++] = strdup("-e");
+	argv[j++] = strdup("trace_imc/trace_cycles/");
+	*argc += 2;
 
-	free(tmp);
 	return 0;
 }
 
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
index 788d216f0852147d..46f9a0adcb608aab 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
@@ -7,7 +7,6 @@
 #include "../../../arch/x86/include/uapi/asm/svm.h"
 #include "../../../arch/x86/include/uapi/asm/vmx.h"
 #include "../../../arch/x86/include/uapi/asm/kvm.h"
-#include <subcmd/parse-options.h>
 
 define_exit_reasons_table(vmx_exit_reasons, VMX_EXIT_REASONS);
 define_exit_reasons_table(svm_exit_reasons, SVM_EXIT_REASONS);
@@ -211,38 +210,15 @@ int __cpu_isa_init_x86(struct perf_kvm_stat *kvm, const char *cpuid)
  */
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv)
 {
-	const char **tmp;
-	bool event = false;
-	int ret = 0, i, j = *argc;
-
-	const struct option event_options[] = {
-		OPT_BOOLEAN('e', "event", &event, NULL),
-		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
-		OPT_END()
-	};
-
-	if (!x86__is_intel_cpu())
-		return 0;
-
-	tmp = calloc(j + 1, sizeof(char *));
-	if (!tmp)
-		return -ENOMEM;
-
-	for (i = 0; i < j; i++)
-		tmp[i] = argv[i];
-
-	parse_options(j, tmp, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
-	if (!event) {
-		argv[j++] = STRDUP_FAIL_EXIT("-e");
-		argv[j++] = STRDUP_FAIL_EXIT("cycles");
-		*argc += 2;
-	}
+	int ret = 0, j = *argc;
+
+	argv[j++] = STRDUP_FAIL_EXIT("-e");
+	argv[j++] = STRDUP_FAIL_EXIT("cycles");
+	*argc += 2;
 
-	free(tmp);
 	return 0;
 
 EXIT:
-	free(tmp);
 	return ret;
 }
 
diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
index 755ab659a05c30f0..c571d2ed711a8345 100644
--- a/tools/perf/util/kvm-stat.c
+++ b/tools/perf/util/kvm-stat.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "debug.h"
+#include "env.h"
 #include "evsel.h"
 #include "kvm-stat.h"
 #include <dwarf-regs.h>
+#include <subcmd/parse-options.h>
 
 bool kvm_exit_event(struct evsel *evsel)
 {
@@ -268,3 +270,42 @@ int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv)
 		return 0;
 	}
 }
+
+bool kvm_need_default_arch_event(uint16_t e_machine, int argc, const char **argv)
+{
+	const char **tmp_argv;
+	bool event = false;
+	int i;
+
+	const struct option event_options[] = {
+		OPT_BOOLEAN('e', "event", &event, NULL),
+		OPT_BOOLEAN(0, "pfm-events", &event, NULL),
+		OPT_END()
+	};
+
+	switch (e_machine) {
+	case EM_PPC:
+	case EM_PPC64:
+		break;
+	case EM_X86_64:
+	case EM_386:
+		if (!x86__is_intel_cpu())
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	/* parse_options() may change the argv, let's make a copy */
+	tmp_argv = calloc(argc + 1, sizeof(char *));
+	if (!tmp_argv)
+		return false;
+
+	for (i = 0; i < argc; i++)
+		tmp_argv[i] = argv[i];
+
+	parse_options(argc, tmp_argv, event_options, NULL, PARSE_OPT_KEEP_UNKNOWN);
+	free(tmp_argv);
+
+	return !event;
+}
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index cdbd921a555f4e51..04f9f3193555972f 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -174,12 +174,20 @@ const char * const *__kvm_skip_events_riscv(void);
 const char * const *__kvm_skip_events_s390(void);
 const char * const *__kvm_skip_events_x86(void);
 
+bool kvm_need_default_arch_event(uint16_t e_machine, int argc, const char **argv);
 int kvm_add_default_arch_event(uint16_t e_machine, int *argc, const char **argv);
 int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv);
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv);
 
 #else /* !HAVE_LIBTRACEEVENT */
 
+static inline bool kvm_need_default_arch_event(uint16_t e_machine __maybe_unused,
+					       int argc __maybe_unused,
+					       const char **argv __maybe_unused)
+{
+	return false;
+}
+
 static inline int kvm_add_default_arch_event(uint16_t e_machine __maybe_unused,
 					     int *argc __maybe_unused,
 					     const char **argv __maybe_unused)
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 2/6] perf kvm: Check kvm_need_default_arch_event() early

[Namhyung Kim]

There's a subtle issue with option parsing in perf record.  It calls the
function with PARSE_OPT_STOP_AT_NON_OPTION so that it can pass later
options to the external command it runs.

But perf kvm record passes the default arch events after the argv.  So
if user calls it with command, then it passes the event to the external
command and fails it like below:

  $ sudo perf kvm --host record sleep 1
  sleep: invalid option -- 'e'
  Try 'sleep --help' for more information.
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.046 MB perf.data.kvm (5 samples) ]

We can check if the default options are needed before passing the extra
command line to make sure it's passed to perf record.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 2f00cc1fd1c7cdbd..24576e4ebb02cade 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1996,13 +1996,14 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 {
 	int rec_argc, i = 0, j, ret;
 	const char **rec_argv;
+	int need_arch_event = !!kvm_need_default_arch_event(EM_HOST, argc, argv);
 
 	/*
 	 * Besides the 2 more options "-o" and "filename",
 	 * kvm_add_default_arch_event() may add 2 extra options,
-	 * so allocate 4 more items.
+	 * so allocate more items conditionally.
 	 */
-	rec_argc = argc + 2 + 2;
+	rec_argc = argc + 2 + (2 * need_arch_event);
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -ENOMEM;
@@ -2010,22 +2011,22 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	rec_argv[i++] = STRDUP_FAIL_EXIT("record");
 	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
 	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
-	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
-
-	BUG_ON(i + 2 != rec_argc);
-
-	if (kvm_need_default_arch_event(EM_HOST, argc, argv)) {
+	if (need_arch_event) {
 		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
 		if (ret)
 			goto EXIT;
 	}
 
+	for (j = 1; j < argc; j++, i++)
+		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+
+	BUG_ON(i != rec_argc);
+
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
+	for (j = 0; j < i; j++)
+		free((void *)rec_argv[j]);
 	free(rec_argv);
 	return ret;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 3/6] perf kvm: Kill STRDUP_FAIL_EXIT()

[Namhyung Kim]

It's used to pass command line options to a copied argv.  But there's no
reason to make the copies as it's all used in the same function.  It can
simply use stack variables.

In fact, it fixes a subtle double free issue.  As parse_options() can
move contents in argv[], some entries may point to the same item.  So
freeing all items in the argv could trigger a double free.  With stack
variables, we don't need to allocate and free them.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c                      | 54 +++++++------------
 .../util/kvm-stat-arch/kvm-stat-powerpc.c     |  4 +-
 tools/perf/util/kvm-stat-arch/kvm-stat-x86.c  |  9 ++--
 tools/perf/util/kvm-stat.h                    | 10 ----
 4 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 24576e4ebb02cade..eaf9297cc34d32c1 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1682,18 +1682,18 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < ARRAY_SIZE(record_args); i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(record_args[i]);
+		rec_argv[i] = record_args[i];
 
 	for (j = 0; j < events_tp_size; j++) {
-		rec_argv[i++] = STRDUP_FAIL_EXIT("-e");
-		rec_argv[i++] = STRDUP_FAIL_EXIT(kvm_events_tp(e_machine)[j]);
+		rec_argv[i++] = "-e";
+		rec_argv[i++] = kvm_events_tp(e_machine)[j];
 	}
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(kvm->file_name);
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = kvm->file_name;
 
 	for (j = 1; j < (unsigned int)argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	set_option_flag(record_options, 'e', "event", PARSE_OPT_HIDDEN);
 	set_option_flag(record_options, 0, "filter", PARSE_OPT_HIDDEN);
@@ -1717,10 +1717,6 @@ kvm_events_record(struct perf_kvm_stat *kvm, int argc, const char **argv)
 
 	record_usage = kvm_stat_record_usage;
 	ret = cmd_record(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2008,9 +2004,9 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	if (!rec_argv)
 		return -ENOMEM;
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("record");
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-o");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
+	rec_argv[i++] = "record";
+	rec_argv[i++] = "-o";
+	rec_argv[i++] = file_name;
 	if (need_arch_event) {
 		ret = kvm_add_default_arch_event(EM_HOST, &i, rec_argv);
 		if (ret)
@@ -2018,15 +2014,13 @@ static int __cmd_record(const char *file_name, int argc, const char **argv)
 	}
 
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	BUG_ON(i != rec_argc);
 
 	ret = cmd_record(i, rec_argv);
 
 EXIT:
-	for (j = 0; j < i; j++)
-		free((void *)rec_argv[j]);
 	free(rec_argv);
 	return ret;
 }
@@ -2041,19 +2035,15 @@ static int __cmd_report(const char *file_name, int argc, const char **argv)
 	if (!rec_argv)
 		return -ENOMEM;
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("report");
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-i");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
+	rec_argv[i++] = "report";
+	rec_argv[i++] = "-i";
+	rec_argv[i++] = file_name;
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	BUG_ON(i != rec_argc);
 
 	ret = cmd_report(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2069,19 +2059,15 @@ __cmd_buildid_list(const char *file_name, int argc, const char **argv)
 	if (!rec_argv)
 		return -ENOMEM;
 
-	rec_argv[i++] = STRDUP_FAIL_EXIT("buildid-list");
-	rec_argv[i++] = STRDUP_FAIL_EXIT("-i");
-	rec_argv[i++] = STRDUP_FAIL_EXIT(file_name);
+	rec_argv[i++] = "buildid-list";
+	rec_argv[i++] = "-i";
+	rec_argv[i++] = file_name;
 	for (j = 1; j < argc; j++, i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[j]);
+		rec_argv[i] = argv[j];
 
 	BUG_ON(i != rec_argc);
 
 	ret = cmd_buildid_list(i, rec_argv);
-
-EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
@@ -2101,7 +2087,7 @@ static int __cmd_top(int argc, const char **argv)
 		return -ENOMEM;
 
 	for (i = 0; i < argc; i++)
-		rec_argv[i] = STRDUP_FAIL_EXIT(argv[i]);
+		rec_argv[i] = argv[i];
 
 	BUG_ON(i != argc);
 
@@ -2114,8 +2100,6 @@ static int __cmd_top(int argc, const char **argv)
 	ret = cmd_top(i, rec_argv);
 
 EXIT:
-	for (i = 0; i < rec_argc; i++)
-		free((void *)rec_argv[i]);
 	free(rec_argv);
 	return ret;
 }
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
index 8d4133c35c12f14b..315b24f9a59297dc 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-powerpc.c
@@ -181,8 +181,8 @@ int __kvm_add_default_arch_event_powerpc(int *argc, const char **argv)
 	if (!perf_pmus__have_event("trace_imc", "trace_cycles"))
 		return -EINVAL;
 
-	argv[j++] = strdup("-e");
-	argv[j++] = strdup("trace_imc/trace_cycles/");
+	argv[j++] = "-e";
+	argv[j++] = "trace_imc/trace_cycles/";
 	*argc += 2;
 
 	return 0;
diff --git a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
index 46f9a0adcb608aab..97041f5100fc812f 100644
--- a/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
+++ b/tools/perf/util/kvm-stat-arch/kvm-stat-x86.c
@@ -210,16 +210,13 @@ int __cpu_isa_init_x86(struct perf_kvm_stat *kvm, const char *cpuid)
  */
 int __kvm_add_default_arch_event_x86(int *argc, const char **argv)
 {
-	int ret = 0, j = *argc;
+	int j = *argc;
 
-	argv[j++] = STRDUP_FAIL_EXIT("-e");
-	argv[j++] = STRDUP_FAIL_EXIT("cycles");
+	argv[j++] = "-e";
+	argv[j++] = "cycles";
 	*argc += 2;
 
 	return 0;
-
-EXIT:
-	return ret;
 }
 
 const char * const *__kvm_events_tp_x86(void)
diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
index 04f9f3193555972f..1db31c0751875875 100644
--- a/tools/perf/util/kvm-stat.h
+++ b/tools/perf/util/kvm-stat.h
@@ -229,14 +229,4 @@ static inline struct kvm_info *kvm_info__new(void)
 	return ki;
 }
 
-#define STRDUP_FAIL_EXIT(s)		\
-	({	char *_p;		\
-		_p = strdup(s);		\
-		if (!_p) {		\
-			ret = -ENOMEM;	\
-			goto EXIT;	\
-		}			\
-		_p;			\
-	})
-
 #endif /* __PERF_KVM_STAT_H */
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 4/6] perf kvm: Do not copy filename string

[Namhyung Kim]

As it removed STRDUP_FAIL_EXIT(), it no longer calls free() for elements
in the copied argv.  Thus, the filename should not be allocated as well.
In fact, it's a pointer to string literals and should be fine to pass
the pointer.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index eaf9297cc34d32c1..4200bf899b51c2a9 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -607,11 +607,11 @@ static const char *get_filename_for_perf_kvm(void)
 	const char *filename;
 
 	if (perf_host && !perf_guest)
-		filename = strdup("perf.data.host");
+		filename = "perf.data.host";
 	else if (!perf_host && perf_guest)
-		filename = strdup("perf.data.guest");
+		filename = "perf.data.guest";
 	else
-		filename = strdup("perf.data.kvm");
+		filename = "perf.data.kvm";
 
 	return filename;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 5/6] perf kvm: Fix a memory leak in the usage string

[Namhyung Kim]

The first element of the kvm_usage string is allocated dynamically by
parse_options_subcommand() if it's NULL.  Callers should free it after
use.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-kvm.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 4200bf899b51c2a9..15a1fb8075de6602 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2135,6 +2135,7 @@ int cmd_kvm(int argc, const char **argv)
 	const char *const kvm_subcommands[] = { "top", "record", "report", "diff",
 						"buildid-list", "stat", NULL };
 	const char *kvm_usage[] = { NULL, NULL };
+	int ret;
 
 	exclude_GH_default = true;
 	perf_host  = 0;
@@ -2153,29 +2154,31 @@ int cmd_kvm(int argc, const char **argv)
 
 		if (!file_name) {
 			pr_err("Failed to allocate memory for filename\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto out;
 		}
 	}
 
 	if (strlen(argv[0]) > 2 && strstarts("record", argv[0]))
-		return __cmd_record(file_name, argc, argv);
+		ret = __cmd_record(file_name, argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("report", argv[0]))
-		return __cmd_report(file_name, argc, argv);
+		ret = __cmd_report(file_name, argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("diff", argv[0]))
-		return cmd_diff(argc, argv);
+		ret = cmd_diff(argc, argv);
 	else if (!strcmp(argv[0], "top"))
-		return __cmd_top(argc, argv);
+		ret = __cmd_top(argc, argv);
 	else if (strlen(argv[0]) > 2 && strstarts("buildid-list", argv[0]))
-		return __cmd_buildid_list(file_name, argc, argv);
+		ret = __cmd_buildid_list(file_name, argc, argv);
 #if defined(HAVE_LIBTRACEEVENT)
 	else if (strlen(argv[0]) > 2 && strstarts("stat", argv[0]))
-		return kvm_cmd_stat(file_name, argc, argv);
+		ret = kvm_cmd_stat(file_name, argc, argv);
 #endif
 	else
 		usage_with_options(kvm_usage, kvm_options);
 
+out:
 	/* free usage string allocated by parse_options_subcommand */
 	free((void *)kvm_usage[0]);
 
-	return 0;
+	return ret;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 6/6] perf test: Simplify perf kvm record/report tests

[Namhyung Kim]

It can now pass command line arguments to perf kvm record --host.
Let's pass 'sleep 1' directly rather than running it in background.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/shell/kvm.sh | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
index f88e859025c42c32..4ca10388748dee93 100755
--- a/tools/perf/tests/shell/kvm.sh
+++ b/tools/perf/tests/shell/kvm.sh
@@ -63,13 +63,7 @@ test_kvm_record_report() {
 	echo "Recording kvm profile for pid ${qemu_pid}..."
 	# Use --host to avoid needing guest symbols/mounts for this simple test
 	# We just want to verify the command runs and produces data
-	# We run in background and kill it because 'perf kvm record' appends options
-	# after the command, which breaks 'sleep' (e.g. it gets '-e cycles').
-	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" &
-	rec_pid=$!
-	sleep 1
-	kill -INT "${rec_pid}"
-	wait "${rec_pid}" || true
+	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" sleep 1
 
 	echo "Reporting kvm profile..."
 	# Check for some standard output from report
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2 1/6] perf kvm: Factor out kvm_need_default_arch_event()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Silent error suppression on memory allocation failure in kvm_need_default_arch_event()
- [Low] Missing POSIX standard header <stdlib.h> for calloc() and free() in kvm-stat.c
--

commit 79c29a62706a76c9c3a62f6592977712a8f8b5fd
Author: Namhyung Kim <namhyung@kernel.org>

perf kvm: Factor out kvm_need_default_arch_event()

The kvm_add_default_arch_event() has a similar logic in each arch to
check if there's an existing command line option for events.  Let's
check it in the generic code and remove the duplication.

> diff --git a/tools/perf/util/kvm-stat.c b/tools/perf/util/kvm-stat.c
> index 755ab659a05c3..c571d2ed711a8 100644
> --- a/tools/perf/util/kvm-stat.c
> +++ b/tools/perf/util/kvm-stat.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "debug.h"
> +#include "env.h"
>  #include "evsel.h"
>  #include "kvm-stat.h"
>  #include <dwarf-regs.h>
> +#include <subcmd/parse-options.h>

[Severity: Low]
Should <stdlib.h> be explicitly included here for calloc() and free() used
later in this file?

Relying on implicit inclusions can lead to compilation failures when building
the perf tool against musl libc.

[ ... ]

> +	/* parse_options() may change the argv, let's make a copy */
> +	tmp_argv = calloc(argc + 1, sizeof(char *));
> +	if (!tmp_argv)
> +		return false;

[Severity: Medium]
Is it intended to silently swallow the allocation error here and return false?

If calloc() fails, returning false conflates a memory allocation failure with
the logical state of the default arch event not being needed. While the caller
might fail on a subsequent allocation, would it be better to propagate the
error so memory pressure conditions are explicitly caught?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623191439.3621593-1-namhyung@kernel.org?part=1

Re: [PATCH v2 4/6] perf kvm: Do not copy filename string

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The error checking logic for `get_filename_for_perf_kvm()` is unreachable dead code.
--

commit 0b8de0bac81d07f5d9beff7d22b8144187910bab
Author: Namhyung Kim <namhyung@kernel.org>

perf kvm: Do not copy filename string

As it removed STRDUP_FAIL_EXIT(), it no longer calls free() for elements
in the copied argv.  Thus, the filename should not be allocated as well.
In fact, it's a pointer to string literals and should be fine to pass
the pointer.

> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 278543ca66a37..021af3c64acd0 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -607,11 +607,11 @@ static const char *get_filename_for_perf_kvm(void)
>  	const char *filename;
>  
>  	if (perf_host && !perf_guest)
> -		filename = strdup("perf.data.host");
> +		filename = "perf.data.host";
>  	else if (!perf_host && perf_guest)
> -		filename = strdup("perf.data.guest");
> +		filename = "perf.data.guest";
>  	else
> -		filename = strdup("perf.data.kvm");
> +		filename = "perf.data.kvm";
>  
>  	return filename;
>  }

[Severity: Low]
Does this change leave unreachable dead code in cmd_kvm()?

Now that get_filename_for_perf_kvm() returns static string literals, it can
never return NULL. However, its caller cmd_kvm() still checks for NULL and
returns -ENOMEM:

	if (!file_name) {
		file_name = get_filename_for_perf_kvm();

		if (!file_name) {
			pr_err("Failed to allocate memory for filename\n");
			return -ENOMEM;
		}
	}

Is it okay to clean up the redundant NULL check and error message from
cmd_kvm() since this allocation failure is no longer possible?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623191439.3621593-1-namhyung@kernel.org?part=4

Re: [PATCH v2 6/6] perf test: Simplify perf kvm record/report tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unwrapped command execution in test script with `set -e` causes premature exit on failure.
--

commit 3ce1271a163998e293f818fbfbc37ed07914f209
Author: Namhyung Kim <namhyung@kernel.org>

perf test: Simplify perf kvm record/report tests

It can now pass command line arguments to perf kvm record --host.
Let's pass 'sleep 1' directly rather than running it in background.

> diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> index f88e859025c42..4ca10388748de 100755
> --- a/tools/perf/tests/shell/kvm.sh
> +++ b/tools/perf/tests/shell/kvm.sh
> @@ -63,13 +63,7 @@ test_kvm_record_report() {
>  	echo "Recording kvm profile for pid ${qemu_pid}..."
>  	# Use --host to avoid needing guest symbols/mounts for this simple test
>  	# We just want to verify the command runs and produces data
> -	# We run in background and kill it because 'perf kvm record' appends options
> -	# after the command, which breaks 'sleep' (e.g. it gets '-e cycles').
> -	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" &
> -	rec_pid=$!
> -	sleep 1
> -	kill -INT "${rec_pid}"
> -	wait "${rec_pid}" || true
> +	perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" sleep 1

[Severity: Medium]
Since this script runs with set -e, will a failure in perf kvm here cause 
the script to exit immediately?

The previous backgrounding implementation handled failures gracefully using
wait "${rec_pid}" || true. If this new foreground command fails (for example 
due to restricted perf_event_paranoid settings), it seems the entire test 
script will abort early and skip the remaining tests instead of accumulating 
errors gracefully. 

Should this be wrapped to catch failures, or append || true to prevent a 
premature script exit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623191439.3621593-1-namhyung@kernel.org?part=6