Re: [PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `__hpp_list__parse()` contains a bug where breaking out of a switch statement fails to terminate the enclosing for-loop, leading to order-dependent error masking.
--

--- Patch [2]: [PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure ---
commit 8cd4f405e8cab7ae300b2dd0a747dc102c29836e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf c2c: Clean up registered formats on c2c_hists__init() failure

When c2c_hists__init() calls hpp_list__parse() and fails partway through,
format structures registered are left on the hpp_list. This adds cleanup
inside c2c_hists__init() itself, so all callers are protected.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..9f6ae3adb7951 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -2063,26 +2065,34 @@ static int c2c_hists__init_sort(struct perf_hpp_list *hpp_list, char *name, stru
>  	return 0;
>  }
>  
> -#define PARSE_LIST(_list, _fn)							\
> -	do {									\
> -		char *tmp, *tok;						\
> -		ret = 0;							\
> -										\
> -		if (!_list)							\
> -			break;							\
> -										\
> -		for (tok = strtok_r((char *)_list, ", ", &tmp);			\
> -				tok; tok = strtok_r(NULL, ", ", &tmp)) {	\
> -			ret = _fn(hpp_list, tok, env);				\
> -			if (ret == -EINVAL) {					\
> -				pr_err("Invalid --fields key: `%s'", tok);	\
> -				break;						\
> -			} else if (ret == -ESRCH) {				\
> -				pr_err("Unknown --fields key: `%s'", tok);	\
> -				break;						\
> -			}							\
> -		}								\
> -	} while (0)
> +static int __hpp_list__parse(struct perf_hpp_list *hpp_list, char *_list, struct perf_env *env,
> +			     int (*_fn)(struct perf_hpp_list *hpp_list, char *name, struct perf_env *env))
> +{
> +	char *tmp, *tok;
> +	int ret = 0;
> +
> +	if (!_list)
> +		return 0;
> +
> +	for (tok = strtok_r(_list, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
> +		ret = _fn(hpp_list, tok, env);
> +		switch (ret) {
> +		case 0:
> +			continue;
> +		case -EINVAL:
> +			pr_err("Invalid --fields key: `%s'", tok);
> +			break;

[Severity: Medium]
Will this break statement only exit the switch block, rather than the enclosing
for-loop?

In the old PARSE_LIST macro, breaking out of the loop aborted parsing upon the
first invalid key.

With a switch statement, it looks like this will continue parsing remaining
tokens, overwriting ret and masking the earlier error if a subsequent token
succeeds.

Does this need to return immediately or use a goto to exit the loop?

> +		case -ESRCH:
> +			pr_err("Unknown --fields key: `%s'", tok);
> +			break;
> +		default:
> +			pr_err("%m for --fields key: `%s'", tok);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}

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