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

[sashiko-bot@kernel.org]

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